-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nextjs): Add SDK to serverside app directory
#6927
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
AbhiPrasad
left a comment
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.
feels... brittle idk. But I guess the next webpack entry code has always been like that.
| loader: path.resolve(__dirname, 'loaders', 'sdkMultiplexerLoader.js'), | ||
| options: { | ||
| importTarget: buildContext.nextRuntime === 'edge' ? './edge' : './client', | ||
| importTarget: { browser: './client', node: './server', edge: './edge' }[runtime], |
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.
l: Let's move this object into it's own const variable?
| // User-specified pages to skip. (Note: For ease of use, `excludeServerRoutes` is specified in terms of routes, | ||
| // which don't have the `pages` prefix.) | ||
| const entryPointRoute = entryPointName.replace(/^pages/, ''); | ||
| if (stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true)) { |
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.
l: can we just do
| if (stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true)) { | |
| return !stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true) |
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.
lol yeah - the classic
| const isomorphicValues = { | ||
| // `rewritesTunnel` set by the user in Next.js config | ||
| __sentryRewritesTunnelPath__: userSentryOptions.tunnelRoute, | ||
| SENTRY_RELEASE: { id: getSentryRelease(buildContext.buildId) }, |
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.
l: can we move the The webpack plugin's release injection breaks the app directory comment down here?
| }); | ||
|
|
||
| it('injects user config file into `_app` in client bundle but not in server bundle', async () => { | ||
| it('injects user config file into `_app` in server bundle but not in client bundle', async () => { |
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.
why did this condition flip?
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.
It basically doesn't matter where we inject what. To clean things up a bit I decided we should inject the server SDK in the pages/_app entrypoint and the client SDK to the main and app-main entrypoints.
I assume this test just verified that we do not accidentally mess this up.
Absolutely. It is horrible but out of the 100 things I've tried, this was the only one that worked reliably... |
Ref: #6726
This PR injects the Next.js SDK into serverside
appdirectory bundles, allowing users to call the Sentry SDK in server components.