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

fix(nextjs): Fix runtime error for static pages #7476

Conversation

baked-dev
Copy link
Contributor

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

We had an issue with this aspect of sentry causing our serverless functions on vercel to error.
The reason is that those pages are statically generated at build-time, where the headers function is never called. However the static page revalidation (according to the revalidate config) resulted in an error since the headers call causes the page to turn from a static into a dynamic page during runtime, which is not allowed by nextjs.
This solution uses the same method next.js uses internally to find out if a request is static and prevents the call to headers in those cases.

@baked-dev
Copy link
Contributor Author

I realized that another, possibly more future proof, solution for this would be to try/catch the headers() call as that would not rely on the undocumented import from next/dist which is likely not covered by semver. Let me know what you think and i'll update the PR accordingly.

@AbhiPrasad
Copy link
Member

possibly more future proof, solution for this would be to try/catch the headers() call as that would not rely on the undocumented import from next/dist which is likely not covered by semver

@baked-dev that sounds more reasonable to me. Feel free to update the PR and then we can merge!

I'll also take care of writing an e2e test for this so we don't regress here again. Appreciate the contribution!

@baked-dev
Copy link
Contributor Author

@AbhiPrasad I updated the code to use try/catch instead. Thank you for taking care of the tests, I was not quite sure if this change was big enough to require additional tests.

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) March 17, 2023 10:00
@AbhiPrasad AbhiPrasad merged commit 90bd190 into getsentry:develop Mar 17, 2023
@AbhiPrasad
Copy link
Member

@baked-dev - this will go out with a release next early next week!

@lforst
Copy link
Member

lforst commented Mar 18, 2023

Thanks for taking care of this. I had this on my short-term to-do list. You implemented it exactly as I would have done with the try-catch block!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants