Skip to content

fix(commonjs): handle global shorthand property correctly#1958

Open
kaigritun wants to merge 1 commit intorollup:masterfrom
kaigritun:fix/commonjs-global-shorthand
Open

fix(commonjs): handle global shorthand property correctly#1958
kaigritun wants to merge 1 commit intorollup:masterfrom
kaigritun:fix/commonjs-global-shorthand

Conversation

@kaigritun
Copy link

Problem

When global is used as a shorthand property in an object literal (e.g., {global}), the commonjs plugin replaces global with commonjsHelpers.commonjsGlobal, resulting in invalid syntax {commonjsHelpers.commonjsGlobal}.

This causes a parse error like:

Unexpected token \`.\`. Expected ... , *, (, [, :, , ?, = or an identifier

Root Cause

As identified in rollup/rollup#6242 by @TrickyPi, the CommonJS plugin transforms {global} into {commonjsHelpers.commonjsGlobal}, which is invalid syntax because you can't have a dotted expression as a shorthand property.

Solution

This fix follows the same pattern already used for the require shorthand case (see existing shorthand-require test): when detecting a shorthand property, prepend global: to convert it to a full property before replacing the value.

The transformation now correctly produces:

  • Before: {global}{commonjsHelpers.commonjsGlobal} (invalid syntax)
  • After: {global}{global: commonjsHelpers.commonjsGlobal} (valid syntax)

Changes

  1. Added shorthand property handling for the global identifier in transform-commonjs.js
  2. Added a test case in test/fixtures/function/shorthand-global/

Related Issues

Fixes rollup/rollup#6242

When `global` is used as a shorthand property in an object literal (e.g.,
`{global}`), the plugin was replacing `global` with
`commonjsHelpers.commonjsGlobal`, resulting in invalid syntax
`{commonjsHelpers.commonjsGlobal}`.

This fix follows the same pattern used for the `require` shorthand case:
when detecting a shorthand property, prepend `global: ` to convert it to a
full property before replacing the value.

Fixes rollup/rollup#6242
@TrickyPi
Copy link
Member

This PR appears to duplicate #1957

@shellscape shellscape removed the request for review from CharlieHelps February 16, 2026 03:52
@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

Pulled this branch and ran pnpm lint + pnpm -r test. Lint is fine, but tests currently fail in @rollup/plugin-commonjs on the new function › shorthand-global fixture with a PARSE_ERROR (Unexpected token '.'), surfaced from commonjs--resolver.

MUST fix

  1. The shorthand-property handling for global still produces invalid JS in practice (the same failure mode described in Cannot build with cssstyle package (issue with object shorthand?) rollup#6242). The current implementation assumes a shorthand property’s key and value are the same AST node (see the inline comment + skippedNodes.add(parent.value)). In this fixture, key/value appear to be distinct nodes sharing a range, so we can end up rewriting the shorthand token into ${helpersName}.commonjsGlobal without reliably expanding it to global: ..., yielding an object literal like { commonjsHelpers.commonjsGlobal } and the parse error. Fix direction: in the shorthand case, expand the property (prependRight at the property/key start), push parent.value into replacedGlobal (not whichever identifier node was visited), and skip parent.key to prevent rewriting the key.
  2. Once the transform is fixed, update/add the snapshot for the new shorthand-global fixture in packages/commonjs/test/snapshots/function.js.md (new fixture dirs change the t.snapshot(codeMap) expectation).

IMPROVEMENTS
3. Consider applying the same “rewrite parent.value explicitly in shorthand” pattern to the existing shorthand require handling as well, so both paths are robust to AST key/value identity differences.
4. Add a fixture for mixed shorthand (e.g. { global, require }) to lock in behavior when both appear in the same object literal.
5. The shorthand-handling blocks for require and global are now very similar—could be factored a bit to reduce copy/paste and future drift.
6. Minor process note: @TrickyPi mentioned this may duplicate #1957; worth confirming whether you want to rebase/close one to avoid parallel fixes.

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.

Cannot build with cssstyle package (issue with object shorthand?)

3 participants