fix(nextjs): Split node export condition into import/require - #22793
fix(nextjs): Split node export condition into import/require#22793Mohith26 wants to merge 3 commits into
node export condition into import/require#22793Conversation
The `node` condition in the root `exports` map of `@sentry/nextjs` was a bare string pointing at the CJS server build. Since `node` matches before the top-level `import` condition for any Node.js process, ESM consumers under a plain Node loader always received the CJS build, and `cjs-module-lexer` could not statically detect the re-exports coming from `@sentry/core` / `@sentry/node`, leaving named imports broken and namespace imports with silently-undefined members. Split the `node` condition into an `import`/`require` pair pointing at the ESM and CJS server builds respectively, matching the shape already used by the sibling `browser`/`edge`/`worker` conditions in this package and by the `node` condition in @sentry/sveltekit and @sentry/nuxt. Also add the explicit `.js` extension to the `next/constants` import so the now-reachable ESM server build is actually loadable by Node's ESM resolver (`next` ships no `exports` map, so extensionless deep imports fail there). Fixes getsentry#22791
Assert that the `node` condition of the root export has an `import`/`require` split whose entries point at files that actually exist in the ESM and CJS build output, and that the ESM server module graph contains no extensionless `next/*` deep imports (which are unresolvable by Node's ESM resolver because `next` ships no `exports` map). Ref getsentry#22791
|
Thank you for your contribution! There are some things that need to be taken into account:
|
The config code (webpack.ts and the turbopack rule builders) resolves loader paths with __dirname, which does not exist in ESM. Derive a module-local dirname from import.meta.url when __dirname is undefined, following the @sentry/bundler-plugins pattern (Rollup transpiles import.meta for the CJS build; the typeof guard keeps CJS on the fast path). Adds a graph test asserting the ESM config build contains no unguarded __dirname references.
|
Thanks for the review! Added the __dirname shim in 33840f5: the config code now derives a module-local dirname from import.meta.url when __dirname is undefined, following the @sentry/bundler-plugins pattern (Rollup transpiles import.meta for the CJS build, and the typeof guard keeps CJS on its fast path). Verified locally: the CJS output contains no bare import.meta, both build/cjs and build/esm config entries load under plain Node, the full package suite passes (906), and a new graph test asserts the ESM config build has no unguarded __dirname. For the E2E side I could not run the nextjs-16-t3 app locally; happy to iterate if the e2e-tests run against this branch surfaces anything. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| import { PHASE_PRODUCTION_BUILD } from 'next/constants'; | ||
| // The explicit `.js` extension is required for the ESM build to be loadable by plain Node.js: `next` does not define | ||
| // an `exports` map, so extensionless deep imports like `next/constants` are not resolvable by Node's ESM resolver. | ||
| import { PHASE_PRODUCTION_BUILD } from 'next/constants.js'; |
There was a problem hiding this comment.
Bug: The import path for next/constants.js in isBuild.ts does not match the Rollup external configuration, which still lists 'next/constants', causing a build failure.
Severity: HIGH
Suggested Fix
Update the external array in packages/nextjs/rollup.npm.config.mjs. Change the entry from 'next/constants' to 'next/constants.js' to match the new import path used in the source code. This will ensure Rollup correctly identifies the module as an external dependency during the build process.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/nextjs/src/common/utils/isBuild.ts#L3
Potential issue: The code was updated to import from `'next/constants.js'` to fix a Node
ESM resolver issue. However, the Rollup configuration in
`packages/nextjs/rollup.npm.config.mjs` was not updated accordingly and still lists
`'next/constants'` as an external dependency. The external matcher function will not
recognize `'next/constants.js'` as external because it neither exactly matches
`'next/constants'` nor starts with `'next/constants/'`. As a result, Rollup will attempt
to bundle this module instead of treating it as external, likely causing a build failure
or incorrect bundling of Next.js internals.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 33840f5. Configure here.
| use: [ | ||
| { | ||
| loader: path.resolve(__dirname, 'loaders', 'wrappingLoader.js'), | ||
| loader: path.resolve(_dirname, 'loaders', 'wrappingLoader.js'), |
There was a problem hiding this comment.
ESM loaders crash on __dirname
High Severity
The node.import split makes ESM next.config resolve @sentry/nextjs to the ESM build, so _dirname now points webpack/turbopack at ESM loaders under build/esm/ (which has "type": "module"). Those loaders still read templates via bare __dirname at module scope, which throws ReferenceError in ESM and breaks builds for ESM apps that previously worked via the old CJS-only node condition.
Reviewed by Cursor Bugbot for commit 33840f5. Configure here.
|
|
||
| expect(visited.size).toBeGreaterThan(1); | ||
| expect(unguardedDirnameUses).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
Dirname test skips loaders
Medium Severity
The new unguarded-__dirname regression test only walks relative imports from the ESM config entry. Loader paths are passed as path.resolve strings, so wrappingLoader and friends are never visited and the test cannot catch the ESM loader crash this PR introduces. Flagged because the review rules require fix-PR tests to actually cover the newly added behaviour.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 33840f5. Configure here.
3 participants
Fixes #22791, reported by @jmatthewpryor.
@sentry/nextjs'snodeexport condition was a bare string pointing at the CJS server build, so ESM consumers under a plain Node loader got the CJS file and a partial namespace: cjs-module-lexer cannot see theexport *re-exports from@sentry/coreand@sentry/node, so named imports likecaptureExceptionwere silentlyundefined.Two changes:
nodecondition intoimport/requireentries pointing at the ESM and CJS server builds, mirroring@sentry/sveltekitand@sentry/nuxtand this package's ownbrowser/edgeconditions..jsextension to thenext/constantsimport inisBuild.ts. Without it the newly reachable ESM build crashes withERR_MODULE_NOT_FOUNDunder plain Node (nextships no exports map; the repo'smoduleResolution: bundlerhid this).Four regression tests verify the split exists, both targets exist in the build output, and the ESM server module graph contains no extensionless
next/*imports. Package suite: 906 passed, 0 failed (baseline 902). oxlint/oxfmt/es-compatibility clean.