-
Notifications
You must be signed in to change notification settings - Fork 347
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: add error page to edge functions for local dev #5070
Feat: add error page to edge functions for local dev #5070
Conversation
📊 Benchmark resultsComparing with 91506bb Package size: 223 MB⬇️ 0.00% decrease vs. 91506bb
Legend
|
d868939
to
483b0b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, but nothing blocking. Great work! ✨
@@ -0,0 +1,289 @@ | |||
<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would it take for us to use the same template for both types of functions? Are they all that different? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some differences right now in some of the texts, and some of the links... I also saw some things in the normal functions template that I'm pretty sure we won't use for the edge functions template, but that I'm not sure about if we use it still for normal functions. So I felt unsure as to combine the two.
I could pass the differences as arguments, but something about that feels off a bit as well because the amount of arguments will grow quite a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've asked some input from the team, if it's all good to clear out some old stuff we don't need anymore I'll merge the two :)
src/utils/proxy.js
Outdated
@@ -386,6 +402,22 @@ const initializeProxy = async function ({ configPath, distDir, port, projectDir | |||
res.setHeader(key, val) | |||
}) | |||
|
|||
const isUncaughtError = responseStatus === 500 && proxyRes.headers['x-nf-uncaught-error'] === '1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check for the status code? I would think that the presence of the header alone should indicate an uncaught error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it's a good check either way, if only from the perspective of readability, as we do set the header ourselves and a 500 is a clear message to the reader that we're dealing with an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that the check is not broad enough. If the bootstrap starts producing an uncaught error with a different status code, the CLI will need to adjust to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O yeah that makes sense :) In that case I will actually change it
c31c31a
to
906fe6d
Compare
Summary
Fixes #4655.
Whenever someone runs
netlify dev
we want people to have a proper error page available for them with a full stacktrace if an edge function has an unhandled error.For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)