Skip to content

fix(codegen): keep exports[STR] = … key as plain string in minify#22402

Merged
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-cjs-exports-computed-target-plain-string
May 15, 2026
Merged

fix(codegen): keep exports[STR] = … key as plain string in minify#22402
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-cjs-exports-computed-target-plain-string

Conversation

@Dunqing

@Dunqing Dunqing commented May 14, 2026

Copy link
Copy Markdown
Member

Stacked on #22400. Closes the second cjs-module-lexer gap.

After #22400, Object.defineProperty(exports, "name", ...) is fine, but exports["name"] = … / module.exports["name"] = … were still being emitted with backtick keys in minify. The lexer recognises this form too — it just couldn't see them past the template literal.

Valid-identifier keys (exports["foo"]) are already collapsed to dot access by the minifier earlier in the pipeline, so only non-identifier keys (exports["has-dash"], module.exports["__esModule"], …) survive to the codegen stage and were affected.

In AssignmentExpression::gen_expr, detect when the LHS is a computed member expression on exports / module.exports whose key is a StringLiteral, and print the key via print_string_literal(key, false). Other computed access (otherObj["x"]) is unaffected.

Where each cjs-module-lexer named-export form lands

Pattern Status
exports.name = … identifier — never affected
module.exports.name = … identifier — never affected
module.exports = { "name": … } object property keys already use allow_backtick: false
Object.defineProperty(exports, "name", …) fixed in #22400
exports["name"] = … / module.exports["name"] = … fixed here

Before / after

// input
exports["has-dash"] = a;
module.exports["__esModule"] = true;
otherObj["not-exports"] = d;
// before
exports[`has-dash`]=a,module.exports[`__esModule`]=!0,otherObj[`not-exports`]=d;

// after
exports["has-dash"]=a,module.exports.__esModule=!0,otherObj[`not-exports`]=d;

(module.exports.__esModule collapses to dot via a separate minifier pass; otherObj["x"] correctly stays backtick.)

Refs #22342

AI assisted.

@github-actions github-actions Bot added the A-codegen Area - Code Generation label May 14, 2026

Dunqing commented May 14, 2026

Copy link
Copy Markdown
Member Author

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of the merge queue

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codspeed-hq

codspeed-hq Bot commented May 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 44 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing fix/codegen-cjs-exports-computed-target-plain-string (decfc18) with main (5ac7e79)

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Dunqing Dunqing force-pushed the fix/codegen-cjs-define-property-plain-string branch from 2b7165a to 05353f5 Compare May 14, 2026 07:06
@Dunqing Dunqing force-pushed the fix/codegen-cjs-exports-computed-target-plain-string branch from 80a9235 to 6f06826 Compare May 14, 2026 07:06
@Dunqing Dunqing marked this pull request as ready for review May 14, 2026 10:25
@Dunqing Dunqing requested a review from Boshen May 14, 2026 10:25
@Dunqing

Dunqing commented May 14, 2026

Copy link
Copy Markdown
Member Author

Same as #22400 (comment) said

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label May 14, 2026

Boshen commented May 14, 2026

Copy link
Copy Markdown
Member

Merge activity

  • May 14, 10:28 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • May 14, 10:29 AM UTC: Boshen added this pull request to the Graphite merge queue.
  • May 14, 10:33 AM UTC: The Graphite merge queue couldn't merge this PR because it had merge conflicts.
  • May 14, 10:36 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • May 14, 10:36 AM UTC: The merge label '0-merge' was removed. This PR will no longer be merged by the Graphite merge queue
  • May 15, 1:54 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • May 15, 1:55 PM UTC: Dunqing added this pull request to the Graphite merge queue.
  • May 15, 1:59 PM UTC: Merged by the Graphite merge queue.

@graphite-app graphite-app Bot changed the base branch from fix/codegen-cjs-define-property-plain-string to graphite-base/22402 May 14, 2026 10:28
@graphite-app graphite-app Bot changed the base branch from graphite-base/22402 to main May 14, 2026 10:32
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label May 14, 2026
@Dunqing Dunqing force-pushed the fix/codegen-cjs-exports-computed-target-plain-string branch from 6f06826 to decfc18 Compare May 15, 2026 13:51
@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label May 15, 2026
…22402)

Stacked on #22400. Closes the second `cjs-module-lexer` gap.

After #22400, `Object.defineProperty(exports, "name", ...)` is fine, but `exports["name"] = …` / `module.exports["name"] = …` were still being emitted with backtick keys in minify. The lexer recognises this form too — it just couldn't see them past the template literal.

Valid-identifier keys (`exports["foo"]`) are already collapsed to dot access by the minifier earlier in the pipeline, so only non-identifier keys (`exports["has-dash"]`, `module.exports["__esModule"]`, …) survive to the codegen stage and were affected.

In `AssignmentExpression::gen_expr`, detect when the LHS is a computed member expression on `exports` / `module.exports` whose key is a `StringLiteral`, and print the key via `print_string_literal(key, false)`. Other computed access (`otherObj["x"]`) is unaffected.

### Where each cjs-module-lexer named-export form lands

| Pattern | Status |
|---|---|
| `exports.name = …` | identifier — never affected |
| `module.exports.name = …` | identifier — never affected |
| `module.exports = { "name": … }` | object property keys already use `allow_backtick: false` |
| `Object.defineProperty(exports, "name", …)` | fixed in #22400 |
| `exports["name"] = …` / `module.exports["name"] = …` | **fixed here** |

### Before / after

```js
// input
exports["has-dash"] = a;
module.exports["__esModule"] = true;
otherObj["not-exports"] = d;
```

```js
// before
exports[`has-dash`]=a,module.exports[`__esModule`]=!0,otherObj[`not-exports`]=d;

// after
exports["has-dash"]=a,module.exports.__esModule=!0,otherObj[`not-exports`]=d;
```

(`module.exports.__esModule` collapses to dot via a separate minifier pass; `otherObj["x"]` correctly stays backtick.)

Refs #22342

AI assisted.
@graphite-app graphite-app Bot force-pushed the fix/codegen-cjs-exports-computed-target-plain-string branch from decfc18 to 703557c Compare May 15, 2026 13:55
@graphite-app graphite-app Bot merged commit 703557c into main May 15, 2026
28 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label May 15, 2026
@graphite-app graphite-app Bot deleted the fix/codegen-cjs-exports-computed-target-plain-string branch May 15, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-codegen Area - Code Generation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants