-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nextjs): Support cacheComponents on turbopack
#18304
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
Conversation
size-limit report 📦
|
cacheComponentscacheComponents on turbopack
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| */ | ||
| function isCacheComponentContext(): boolean { | ||
| return process.env.NEXT_PHASE === 'phase-production-build'; | ||
| } |
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.
So right now now, all startSpan operations performed by users will be using the non-recording spans?
I suppose that means they can't manually instrument anything until the heuristic gets changed?
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.
This will be phase-production-build only during the build. During runtime tracing works as usual. Our tracing did not have any effect during build-time anyway since instrumentation.ts does not get registered – but we still ran into the uuid calls then
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.
Thanks, that makes a lot of sense 👍
| export { captureUnderscoreErrorException } from '../common/pages-router-instrumentation/_error'; | ||
|
|
||
| // Override core span methods with Next.js-specific implementations that support Cache Components | ||
| export { startSpan, startSpanManual, startInactiveSpan } from '../common/utils/nextSpan'; |
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.
A more robust change here might be to set a different acs for nextjs when you initialize the sdk server-side.
That means people can still use imports from @sentry/core or @sentry/node if they want, which might be useful for us.
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.
But we would not call init in this case (of a build)
|
@logaretm @AbhiPrasad I added a better check now to both verify that we're:
also updated the tests for these two scenarios |
With this PR we stop breaking builds by simply re-exporting APIs that make use of synchronous random APIs:
startSpanstartSpanManualstartInactiveSpanWe do this with a naive approach of detecting a prod build phase and returning a noop span in cases where we call these APIs. We hope to get a more solid API for this from Vercel at some point so we can make this more bulletproof. The current version will also still emit a warning in webpack as the behaviour seems to change there.
This will only work for Turbopack as of now, as we still need to get rid of build time instrumentation in webpack in order for this to work.
closes #17895