fix: guard require.extensions for Node 24.15+ under Yarn PnP#93248
Open
bmorros94 wants to merge 4 commits intovercel:canaryfrom
Open
fix: guard require.extensions for Node 24.15+ under Yarn PnP#93248bmorros94 wants to merge 4 commits intovercel:canaryfrom
bmorros94 wants to merge 4 commits intovercel:canaryfrom
Conversation
On Node 24.15+, the `require` function passed to CJS modules loaded via the ESM loader no longer has `.extensions` defined when the module source comes through a custom loader (Yarn PnP's zip loader). This causes `next build` to crash with a TypeError at module load time. Add optional chaining on the top-level read and early-return guards in registerHook/deregisterHook so the file loads safely when require.extensions is undefined. No behavior change on Node versions where require.extensions is defined. Fixes vercel#92935
Add a unit test that simulates the Node 24.15+ Yarn PnP environment by clearing Module._extensions before the hook module is loaded. The fresh require created inside `jest.isolateModules` then sees `require.extensions` as undefined, exercising the new guards in `registerHook` and `deregisterHook`. The previously crashing module-level optional chain (`oldJSHook = require.extensions?.['.js']`) is also covered. A second describe block covers the standard Node 20/22 / non-PnP path: both functions must continue to return without throwing. We do not assert on which handlers got registered because Jest gives each isolated require its own `extensions` object distinct from `Module._extensions`, so we cannot inspect them from the test side; the actual handler wiring is exercised by the existing next-config-ts integration tests.
736ca6b to
bb65567
Compare
…ce tautological tests with real subprocess regression coverage
require-hook.ts:
- Add a `warnOnce` inside the `if (!require.extensions)` early-return so
users with `require('./helper.ts')` in next.config.ts on Node 24.15+
Yarn PnP get an actionable diagnostic instead of a downstream
cryptic `TypeError`.
- Hoist the `oldJSHook` undefined check into the same early-return and
drop the `oldJSHook!` non-null assertions. TypeScript can now narrow
oldJSHook to a callable inside the closures, removing the runtime
hazard of calling an undefined hook if the env transitioned.
require-hook.test.ts:
- The previous `Module._extensions = undefined` + `jest.isolateModules`
simulation does not exercise the regression path. Jest's
`_createRequireImplementation` unconditionally sets
`moduleRequire.extensions = Object.create(null)` (a truthy empty
object), so the SUT always sees a defined extensions object regardless
of the outer process state. The "Node 24.15+ Yarn PnP" tests passed
on canary too — they were tautological smoke tests.
- Replace them with subprocess tests that load the SUT inside a `vm`
wrapper using a custom require whose `.extensions` is genuinely
undefined, faithfully matching Yarn PnP's bridged require. Verified
these tests fail on the unfixed source (TypeError reading '.js') and
pass on the fixed source.
- Assert the new `warnOnce` fires from the registerHook subprocess test.
- Keep the existing jest-process tests as happy-path smoke coverage and
document the limitation in a header comment.
Author
|
This still applies cleanly to current canary from my local check. Could a maintainer please approve CI and review when you have a chance? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
require.extensionsaccesses inpackages/next/src/build/next-config-ts/require-hook.tsagainstundefinedrequirefunction passed to CJS modules loaded via the ESM loader no longer has.extensionswhen the module source comes through a custom loader (Yarn PnP's zip loader), causingnext buildto crash with aTypeErrorat module load time?.) on the top-level read and early-return guards inregisterHook/deregisterHookso the file loads safely whenrequire.extensionsis undefinedrequire.extensionsis definedFixes #92935
Test plan
packages/next/src/build/next-config-ts/require-hook.test.tscovering:oldJSHook = require.extensions?.['.js']) does not throw whenrequire.extensionsis undefinedregisterHook(swcOptions)returns without throwing on the undefined path (Node 24.15+ Yarn PnP simulation viaModule._extensions = undefined+jest.isolateModules)deregisterHook()returns without throwing on the undefined pathregisterHook/deregisterHookcontinue to return without throwing on the standard Node 20/22 / non-PnP pathpnpm jest packages/next/src/build/next-config-ts/require-hook.test.ts)next buildworks on Node 24.15+ with Yarn PnP (the crash scenario from the issue)next buildstill works on Node 20/22 with Yarn PnP (no regression)