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): Stop excluding withSentryConfig from serverless bundles #6267

Merged

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Nov 23, 2022

In #6058, we modified the webpack config we apply to users' builds in the nextjs SDK in order to exclude build-time files from being included in the dependency manifests nextjs creates for each page file, which in turns prevents them from being bundled in the serverless functions into which vercel turns users' routes. Unfortunately, we were just a touch too aggressive, in that we excluded withSentryConfig.ts, even though it's needed at runtime when nextjs loads next.config.js.

This fixes that by making two key changes:

  • Instead of excluding withSentryConfig.ts and all of its dependencies, we instead exclude webpack.ts and dependencies.
  • In order that withSentryConfig.ts not try to require webpack.ts at runtime (when it won't be there), we now conditionally require it, only at buildtime.

Notes:

  • Since the repeated logic in withSentryConfig was about to get longer, I split it out into a helper function before adding in the conditional require call.
  • Because webpack.ts is now only conditionally required, rollup doesn't automatically pick it up, so it's been added explicitly to the nextjs rollup config.
  • In order to make it possible to test whether or not webpack.ts has been required, it now sets an env variable noting that it's been loaded.
  • In the aforementioned PR, index.ts was renamed to withSentryConfig.ts. This PR does the same thing for the corresponding test file.

Fixes #6265.

@lforst lforst self-requested a review November 23, 2022 09:16
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved to unblock but the yarn.lock change should be adressed!

// nft file manifests nextjs generates. (See the code dealing with the `TraceEntryPointsPlugin` below.) So that we don't
// crash people, we therefore need to make sure nothing tries to either require this file or import from it. Setting
// this env variable allows us to test whether or not that's happened.
process.env.SENTRY_WEBPACK_MODULE_LOADED = 'true';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l/m: I just wanna mention that this doesn't actually set an environment variable. This just places the SENTRY_WEBPACK_MODULE_LOADED property on the global process.env object - which is more or less the same as doing global.SENTRY_WEBPACK_MODULE_LOADED = true. It's just that the child_process api will pick up the process.env object if not configured otherwise.

I don't know if we want to make that differentiation to avoid ambiguity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I don't totally see the distinction you're making. process.env is the env for the running node process. Sure, there's an env in the shell in which the script is running, but AFAIK there's no way to modify that from within node, so I don't think that's how anyone would interpret this comment, do you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just set a global variable here, then we can also use a boolean instead of a string.

Setting on process.env has some behaviours that maybe someone might not realize, on windows things become case insensitive (process.env.FOO === process.env.foo) and everything on process.env gets coerced to strings when the getter is called.

Since we don't need the above two behaviours, I would rather just set a mutable global. More obvious what is happening to users, and also allows us to lift the comment up from the usage of the variable to the declaration of the variable (cleaning up the function scope a little).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering Abhi I changed it to be a global in 728562b

"@sentry/core" "7.20.0"
"@sentry/types" "7.20.0"
"@sentry/utils" "7.20.0"
"@sentry/browser@7.20.1":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h: We should merge master into this branch. Yarn lock shouldn't have any changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the test app in the build process tests, not our main yarn.lock. I think I get why Kamil set his integration test app to not use a lockfile, because this is going to happen every time we bump a version, but that's handlable in a separate PR. I can pull this bit out of this PR if you feel strongly about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I am dumb sorry

// to test whether or not the file is required instead.

it('imports from `webpack.ts` if `isBuild` returns true', () => {
jest.isolateModules(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

til

@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-stop-excluding-withSentryConfig-from-nft-files branch from 0b68c8f to 52faf56 Compare November 23, 2022 10:08
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-stop-excluding-withSentryConfig-from-nft-files branch from 52faf56 to a9af4aa Compare November 23, 2022 10:09
@lforst lforst merged commit b35e3d7 into master Nov 23, 2022
@lforst lforst deleted the kmclb-nextjs-stop-excluding-withSentryConfig-from-nft-files branch November 23, 2022 10:58
@isaac-martin
Copy link

Hey something in this release has meant that our application stopped sending events, trying to track it down further

@lforst
Copy link
Member

lforst commented Dec 8, 2022

@isaac-martin Is this also the case for the newest version of the SDK? In the release that included this PR, we broke the sending of events in dev mode, but that should be fixed in 7.24.0.

@hiramhuang
Copy link

hiramhuang commented Dec 9, 2022

My application stopped sending events after I upgraded to @sentry/nextjs@7.21.1. I wonder if maybe it's related to this PR?

✅ = Works fine
❌ = Not sending events, also no debugging messages.

7.13.0 ✅ (before upgrade)
7.14.0 ✅
7.15.0 ✅
7.17.4 ✅
7.18.0 ✅
7.20.0 ✅
7.21.0 ✅
7.21.1 ❌
7.24.0 ❌
7.24.2 ❌

@lforst
Copy link
Member

lforst commented Dec 9, 2022

@hiramhuang Hi, could you open an issue describing your problem with logs and your next.js configuration?

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.

Sentry is working on my local but not on production deployed in vercel
5 participants