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’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

Merged
merged 9 commits into from Oct 3, 2022

Conversation

khendrikse
Copy link
Contributor

@khendrikse khendrikse commented Sep 19, 2022

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:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)
cat sneezing

@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Sep 19, 2022
@github-actions
Copy link

github-actions bot commented Sep 19, 2022

📊 Benchmark results

Comparing with 91506bb

Package size: 223 MB

⬇️ 0.00% decrease vs. 91506bb

^  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

@khendrikse khendrikse changed the title Feat/4655/add error page to edge functions dev Feat: add error page to edge functions for local dev Sep 22, 2022
@khendrikse khendrikse force-pushed the feat/4655/add-error-page-to-edge-functions-dev branch from d868939 to 483b0b0 Compare September 22, 2022 11:34
@khendrikse khendrikse self-assigned this Sep 22, 2022
@khendrikse khendrikse marked this pull request as ready for review September 22, 2022 11:35
@khendrikse khendrikse requested a review from a team September 22, 2022 11:35
eduardoboucas
eduardoboucas previously approved these changes Sep 23, 2022
Copy link
Member

@eduardoboucas eduardoboucas left a 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! ✨

src/lib/render-error-remplate.js Outdated Show resolved Hide resolved
@@ -0,0 +1,289 @@
<!DOCTYPE html>
Copy link
Member

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? 🤔

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 Show resolved Hide resolved
@@ -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'
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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

src/utils/proxy.js Show resolved Hide resolved
eduardoboucas
eduardoboucas previously approved these changes Sep 23, 2022
eduardoboucas
eduardoboucas previously approved these changes Sep 23, 2022
@khendrikse khendrikse merged commit b9e0917 into main Oct 3, 2022
@khendrikse khendrikse deleted the feat/4655/add-error-page-to-edge-functions-dev branch October 3, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring netlify-branded functions error page to edge functions
2 participants