Skip to content

fix(codegen): keep Object.defineProperty property name as plain string in minify#22400

Merged
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-cjs-define-property-plain-string
May 14, 2026
Merged

fix(codegen): keep Object.defineProperty property name as plain string in minify#22400
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-cjs-define-property-plain-string

Conversation

@Dunqing

@Dunqing Dunqing commented May 14, 2026

Copy link
Copy Markdown
Member

Fix cjs-module-lexer failing to detect Object.defineProperty(exports, "name", ...) named exports after oxc minify.

calculate_quote_maybe_backtick favored backtick when all three quote costs tied at zero, so a plain ASCII property name like "getInclusionReasons" was emitted as a backtick template. cjs-module-lexer (used by Node for CJS/ESM interop) only matches plain string literals there, so the affected exports silently dropped out of its export set — downstream ESM consumers then failed with Named export 'X' not found.

In CallExpression::gen_expr, detect Object.defineProperty / Reflect.defineProperty and print the 2nd argument via print_string_literal(name, false) (the existing allow_backtick: false path). Other arguments — and nested strings inside the descriptor — still benefit from backtick optimization. Minify-only: outside minify, print_string_literal uses self.quote (never backtick), and print_arguments is needed there to preserve argument comments.

Before / after

// input
Object.defineProperty(exports, "getInclusionReasons", { ... });
// before
Object.defineProperty(exports,`getInclusionReasons`,{...});

// after
Object.defineProperty(exports,"getInclusionReasons",{...});

Closes #22342

AI assisted.

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

✅ 48 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing fix/codegen-cjs-define-property-plain-string (05353f5) with main (1661122)

Open in CodSpeed

Footnotes

  1. 3 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 marked this pull request as ready for review May 14, 2026 10:22
@Dunqing

Dunqing commented May 14, 2026

Copy link
Copy Markdown
Member Author

If we must continue using backtick for smaller output, we should limit its use to the cases that cjs-module-lexer can handle. This would make the implementation somewhat strange.

@Dunqing Dunqing requested a review from Boshen May 14, 2026 10:24
@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

…ing in minify (#22400)

Fix `cjs-module-lexer` failing to detect `Object.defineProperty(exports, "name", ...)` named exports after oxc minify.

`calculate_quote_maybe_backtick` favored backtick when all three quote costs tied at zero, so a plain ASCII property name like `"getInclusionReasons"` was emitted as a backtick template. [`cjs-module-lexer`](https://github.com/nodejs/cjs-module-lexer) (used by Node for CJS/ESM interop) only matches plain string literals there, so the affected exports silently dropped out of its export set — downstream ESM consumers then failed with `Named export 'X' not found`.

In `CallExpression::gen_expr`, detect `Object.defineProperty` / `Reflect.defineProperty` and print the 2nd argument via `print_string_literal(name, false)` (the existing `allow_backtick: false` path). Other arguments — and nested strings inside the descriptor — still benefit from backtick optimization. Minify-only: outside minify, `print_string_literal` uses `self.quote` (never backtick), and `print_arguments` is needed there to preserve argument comments.

### Before / after

```js
// input
Object.defineProperty(exports, "getInclusionReasons", { ... });
```

```js
// before
Object.defineProperty(exports,`getInclusionReasons`,{...});

// after
Object.defineProperty(exports,"getInclusionReasons",{...});
```

Closes #22342

AI assisted.
@graphite-app graphite-app Bot force-pushed the fix/codegen-cjs-define-property-plain-string branch from 05353f5 to 18edc2c Compare May 14, 2026 10:28
@graphite-app graphite-app Bot merged commit 18edc2c into main May 14, 2026
28 checks passed
@graphite-app graphite-app Bot deleted the fix/codegen-cjs-define-property-plain-string branch May 14, 2026 10:32
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label May 14, 2026
graphite-app Bot pushed a commit that referenced this pull request 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.
overlookmotel added a commit that referenced this pull request May 15, 2026
### 🚀 Features

- bc91a17 codegen: Expose `Codegen::with_source_type` method (#22432)
(camc314)

### 🐛 Bug Fixes

- 5ac7e79 minifier: Drop unused-var-init pure IIFEs and preserve
annotation for downstream (#22349) (Dunqing)
- 4ab57eb allocator: Fixed-size allocators use `VirtualAlloc` on Windows
(#22124) (overlookmotel)
- 66d77eb allocator: Fix segfault on Linux MUSL with fixed-size
allocators (#22388) (overlookmotel)
- b8fbc1f transformer/object-rest-spread: Correct scope id when moving
bindings (#22419) (camc314)
- 18edc2c codegen: Keep `Object.defineProperty` property name as plain
string in minify (#22400) (Dunqing)
- dda33de transformer/explicit-resource-management: Align lexical
binding scopes (#22320) (camc314)
- 8e79de8 transformer: Preserve for-await statement bodies (#22361)
(camc314)
- 0cba210 transformer/class: Replace `new.target` in static blocks
(#22360) (camc314)
- 67ab1c9 transformer/es2018/for-await: Hoist for-await generated
bindings (#22355) (camc314)
- c3ceb4a transformer/object-rest-spread: Use hoisted scope for `for-of`
temp refs (#22347) (camc314)

### ⚡ Performance

- 73a9043 allocator/bitset: Avoid temp heap `String` allocation (#22403)
(camc314)
- 8b2f4f9 transformer/object-rest-spread: Collect `Vec<SymbolId` over
`Vec<BindingIdentifier>` (#22418) (camc314)
- 83679ea parser: Split TriviaBuilder::handle_token hot/cold paths
(#22415) (Boshen)
- 2c7d781 codegen: Inline identifier-name accessors (#22411) (Boshen)
- 618bc76 diagnostics: Inline `OxcDiagnosticInner` to avoid heap
allocation (#22406) (Boshen)
- 0b4e158 parser: Reserve cap `2` for sequence expressions vec (#22374)
(camc314)
- 5f3bdd0 codegen: Add `#[inline]` to `code`, `code_len` (#22373)
(camc314)

Co-authored-by: overlookmotel <557937+overlookmotel@users.noreply.github.com>
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.

minifier: Object.defineProperty(exports, "name", ...) key emitted as a template literal breaks cjs-module-lexer

2 participants