Skip to content

fix(commonjs): correctly replaces shorthand "global" property in object#1957

Open
TrickyPi wants to merge 1 commit intorollup:masterfrom
TrickyPi:fix/handle-shorthand-global
Open

fix(commonjs): correctly replaces shorthand "global" property in object#1957
TrickyPi wants to merge 1 commit intorollup:masterfrom
TrickyPi:fix/handle-shorthand-global

Conversation

@TrickyPi
Copy link
Member

@TrickyPi TrickyPi commented Jan 26, 2026

Rollup Plugin Name: @rollup/plugin-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 rollup/rollup#6242 (comment)

Description

@TrickyPi TrickyPi requested a review from shellscape as a code owner January 26, 2026 05:53
@TrickyPi TrickyPi changed the title Correctly replaces shorthand "global" property in object fix(commonjs): correctly replaces shorthand "global" property in object Jan 26, 2026
@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 went through the diff + new fixture/snapshots and checked the PR’s CI status; this looks ready to merge from my side.

The change correctly detects an object literal shorthand property { global } and expands it to an explicit key/value ({ global: … }) before rewriting the global identifier to the CommonJS helper. That avoids the invalid output that was causing the parse error reported in rollup/rollup#6242.

IMPROVEMENTS

  1. Consider adding a few small fixtures to lock in nearby behavior and prevent regressions:
    • non-shorthand: { global: global }
    • quoted key: { 'global': global }
    • shadowed binding (should NOT rewrite): function f(global) { return { global }; }
    • destructuring binding (should NOT rewrite): const { global } = x
  2. Minor review ergonomics: packages/commonjs/test/snapshots/function.js.snap shows as a binary diff in GitHub — if there’s an easy way to make that a text diff (line endings / encoding), it’ll be easier to audit in the future.

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?)

2 participants