Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FW-1706] Refactoring of various error-related matters on SF #2871

Merged
merged 3 commits into from Oct 26, 2023

Conversation

sebaholesz
Copy link
Contributor

@sebaholesz sebaholesz commented Oct 11, 2023

Q A
Description, reason for the PR This PR mainly aims to improve how we work with status codes and how we understand error handling.
New feature No
BC breaks No
Fixes issues ...
Have you read and signed our License Agreement for contributions? Yes

馃寪 Live Preview:

@sebaholesz sebaholesz self-assigned this Oct 11, 2023
@sebaholesz sebaholesz added the DX & Refactoring Requests for DX improvements and refactorings label Oct 11, 2023
@sebaholesz sebaholesz changed the title Sh serverside error codes [FW-1706] Refactoring of various error-related matters on SF Oct 11, 2023
Copy link
Contributor

@vitek-rostislav vitek-rostislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback from testing:

  • resolve-friendly-url script can hypothetically return any return code so I suggest taking this into account in middleware.ts - I would respect the returned code and pass it in MIDDLEWARE_STATUS_CODE_KEY. For 4XX codes, it can still return the message 'Friendly URL page not found'
  • middleware.ts - It is not safe to pass the JSON.stringify(e) in the MIDDLEWARE_STATUS_MESSAGE_KEY header as it might expose sensitive data. I would rather use the logException function.
  • middleware.ts - the JSON.stringify(e) piece of code mentioned above does not work as expected - in the logs, I see just Middleware runtime error ({}}) - but this should be probably resolved by the point above
  • It would be nice to describe the logException function in the documentation and motivate developers to use it

@vitek-rostislav
Copy link
Contributor

Hi, @sebaholesz, thanks for the fixes 馃憣 I noticed one more thing that might be addressed (but I do not insist on that, I suggest you discuss this with @pk16011990. We have the following code in _error.tsx:

const ErrorPage: NextPage<ErrorPageProps> = ({ hasGetInitialPropsRun, err, statusCode }): ReactElement => {
    if (!hasGetInitialPropsRun && err) {
        logException(err);
    }

    return statusCode === 404 ? <Error404Content /> : <Error500Content />;
};

That means, even 410 errors (which now can be returned from the middleware) will be rendered as 500 error page. So the page currently looks like this:
image

It is essential to mention that the HTTP code is correctly set to 410.
For me, a good enough solution would be:

- return statusCode === 404 ? <Error404Content /> : <Error500Content />;
+ const notFoundStatusCodes = [404, 410];
    
+ return notFoundStatusCodes.includes(statusCode) ? <Error404Content /> : <Error500Content />;

However, the question is, what about the other status codes, do we want to care about them? 馃檪

Copy link
Member

@pk16011990 pk16011990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing upgrade notes

- previous state contained unnecessary duplicit logging
and similar problems
- it was also unclear where the problems are handled
- the current code was stripped of unnecessary things
and the behavior has been described in a new section
of the docs
@sebaholesz sebaholesz merged commit 08e6d5a into 14.0 Oct 26, 2023
17 checks passed
@sebaholesz sebaholesz deleted the sh-serverside-error-codes branch October 26, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX & Refactoring Requests for DX improvements and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants