Skip to content

fix(commonjs): initialize replaced module.exports to {} for circular requires - #2045

Open
ItsRenjiii wants to merge 2 commits into
rollup:masterfrom
ItsRenjiii:fix/commonjs-replace-exports-init
Open

ItsRenjiii wants to merge 2 commits into
rollup:masterfrom
ItsRenjiii:fix/commonjs-replace-exports-init

Conversation

@ItsRenjiii

@ItsRenjiii ItsRenjiii commented Nov 7, 2026 •

Copy link
Copy Markdown

Rollup Plugin Name: commonjs

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Reported in discussion #2043 (no issue filed).

Description

A module whose only export is a top-level module.exports = ... assignment uses exportMode === 'replace'. When it also gets a require wrapper, its exports variable is declared with no value:

var a;
var hasRequiredA;

function requireA () {
	if (hasRequiredA) return a;
	hasRequiredA = 1;
	const b = requireB();
	a = { seenByB: b.seen };
	return a;
}

A circular require that reaches the module before the assignment runs therefore gets undefined, where Node gives it the initial {} exports object. A top-level this used before the assignment is also undefined, so this.x = 1 throws TypeError: Cannot set properties of undefined.

This PR gives the variable Node's initial {} in both places it can be read early:

  • With a require wrapper, a = {}; is set when the first require starts, right after the hasRequiredA guard. Setting it there rather than at module level keeps a value that a re-entrant require already completed: in a mixed ES/CommonJS cycle a module-level var a = {}; can run after requireA() has finished and erase its exports.
  • Without a require wrapper (strictRequires: false, or 'auto'/'debug' for a module outside a cycle), top-level this is rewritten to a and runs before var a = .... When the module uses top-level this, it now gets var a = {}; before the body, and the assignment becomes a plain a = .... Other unwrapped modules produce the same output as before.

The module.exports = assignment still replaces the object once it runs.

Reproduction from the discussion (rollup 4.68.0, @rollup/plugin-commonjs 29.0.3):

node entry.js bundle before bundle after
circular require (strictRequires: true / 'auto') { seenByB: 'object' } { seenByB: 'undefined' } { seenByB: 'object' }
this.x = 1 before module.exports = (true / 'auto' / false) { y: 2 } TypeError { y: 2 }

A cycle with strictRequires: false still gives undefined, as on master, because that mode does not support cycles.

Tests

  • New function fixtures:
    • strict-requires-circular-replace (cycle) and strict-requires-this-before-replace, this-before-replace-auto and this-before-replace-unwrapped (top-level this under true, 'auto' and false). All four fail on master.
    • strict-requires-circular-replace-mixed: the mixed ES/CommonJS cycle from review. It fails on this PR's first commit and passes now.
  • The updated form outputs and snapshots change only var x; + x = {}; inside the require function, plus one chunk hash that follows from its content.
  • The steps from validate.yml all pass locally: pnpm install --frozen-lockfile, prettier:check, pnpm --recursive build, lint:js and ci:coverage for ...[origin/master]. The commonjs suite (296 tests) passes on Node 20 and 24.

🤖 Generated with Claude Code

…requires

When a module's only export is a top-level `module.exports = ...` assignment
and it is wrapped in a require function, its exports variable was declared
without a value. A circular require (or a top-level `this`) that ran before the
assignment then saw `undefined` instead of the initial `{}` exports object that
Node provides.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; both previous failures are addressed.

Reviews (2) · Last reviewed commit: "fix(commonjs): set replaced exports on f..." · Reviewed by Greptile

…evel this

Address review feedback:

- With a require wrapper, assign `{}` when the first require starts instead of
  initializing the variable at module level. In a mixed ES/CommonJS cycle a
  re-entrant require can complete the module before its body resumes, and the
  module-level initializer then overwrote the completed exports.
- Without a require wrapper (`strictRequires: false`, or `'auto'`/`'debug'` for
  modules outside a cycle), declare `var exports = {}` before the module body
  when it uses top-level `this`, which is rewritten to that variable and can run
  before the `module.exports =` assignment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant