Skip to content

Fix/commonjs conditional exports#1952

Open
sodiumjoe wants to merge 3 commits intorollup:masterfrom
sodiumjoe:fix/commonjs-conditional-exports
Open

Fix/commonjs conditional exports#1952
sodiumjoe wants to merge 3 commits intorollup:masterfrom
sodiumjoe:fix/commonjs-conditional-exports

Conversation

@sodiumjoe
Copy link

@sodiumjoe sodiumjoe commented Dec 5, 2025

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:

resolves #1771

Description

This fix resolves a bug where conditional exports in LogicalExpression (||) with falsy right operands are not properly transformed, causing "ReferenceError: exports is not defined" in ES module builds.

@sodiumjoe sodiumjoe requested a review from shellscape as a code owner December 5, 2025 00:35
@shellscape
Copy link
Collaborator

@sodiumjoe please push a new commit to your branch so the github (buggy) approve workflows run UX will get triggered again, and I can approve the run

@shellscape
Copy link
Collaborator

@CharlieHelps please do a thorough review of this PR to determine if it's ready to merge. Look at the diff, reviews, comments, and referenced issues/docs/links to build a complete understanding. Then respond with a comment that includes a list of MUST fix items that must be fixed before the the PR is merged, and a list of IMPROVEMENTS that shouldn't block the merge but may be quick improvements or things that could be addressed in a follow up PR. The MUST fix items should explain exactly what's wrong and what needs to change to resolve the problem. Both should be numbered lists and the IMPROVEMENTS list should continue counting up from the MUST list. It's OK if there are no IMPROVEMENTS or MUST fix items, you can just omit the lists.

@charliecreates
Copy link
Contributor

I pulled the branch locally and reviewed the diff + new fixtures. Local run: pnpm -C packages/commonjs test ✅ (290 tests), and pnpm -w lint:js ✅.

MUST fix

  1. packages/commonjs/src/ast-utils.js: the new tri-state operators['||'] implementation fixes the unknown || false misclassification, but it regresses the unknown || true case. Today isTruthy(condition || true) returns null, even though condition || true is always truthy. This makes transform-commonjs treat if (condition || true) ... else ... as conditional and prevents dead-branch skipping, which can change output (extra traversal/wrapping and potentially collecting exports from unreachable code). Fix: treat a definitely-truthy RHS as definitely truthy (e.g. if (leftTruthy === true || rightTruthy === true) return true; ...) and update the conditional-exports-or-true fixture expectation accordingly.
  2. No CI checks are showing up on the PR branch right now (gh pr checks reports none). Before merge we should get the normal GitHub Actions checks to run and pass (push a no-op commit / rebase if needed to retrigger, per the earlier maintainer comment).

IMPROVEMENTS
3. Consider extracting a small helper for tri-state boolean folding (for both || and &&) and adding a couple focused unit/fixture cases, so future tweaks don’t accidentally reintroduce null-vs-false mistakes.
4. A short comment near isTruthy/isFalsy documenting the return contract (true/false/null = unknown) would make these optimizations less error-prone.

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.

[plugin-commonjs] Untransformed exports in conditional branch

3 participants