-
-
Notifications
You must be signed in to change notification settings - Fork 720
fix(codegen): avoid backticks for object property keys in destructuring assignments #13631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(codegen): avoid backticks for object property keys in destructuring assignments #13631
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via 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. |
|
@copilot add tests for the following cases ({ "test-key": key });
(class { "test-key" = key }); |
CodSpeed Instrumentation Performance ReportMerging #13631 will not alter performanceComparing Summary
Footnotes |
... Added tests for both requested cases:
Both test cases now verify that string literal property keys are properly handled with double quotes during minification. (c9bccc7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a codegen issue where destructuring assignments with string literal property keys (especially hyphenated ones) were incorrectly using backticks instead of double quotes during minification.
- Added explicit handling for
PropertyKey::StringLiteralinAssignmentTargetPropertyProperty - Ensured string literals use double quotes by setting
allow_backticktofalse - Added comprehensive test coverage for various destructuring scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/oxc_codegen/src/gen.rs | Added explicit PropertyKey::StringLiteral case to prevent backtick usage in destructuring assignments |
| crates/oxc_codegen/tests/integration/js.rs | Added test cases covering destructuring with string literal keys and mixed property types |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Merge activity
|
…ng assignments (#13631) Fixes #13558 ## Problem When minifying JavaScript code with destructuring assignments that contain string literal property keys (especially those with hyphens), the codegen was incorrectly using backticks instead of double quotes: ```javascript // Input ({"aria-label": ariaLabel} = input); // Incorrect output (before fix) ({`aria-label`:ariaLabel}=input); // Correct output (after fix) ({"aria-label":ariaLabel}=input); ``` ## Root Cause The issue was in the `AssignmentTargetPropertyProperty` implementation in `crates/oxc_codegen/src/gen.rs`. Unlike the regular `PropertyKey` implementation and other similar constructs (`TSMethodSignature`, `TSPropertySignature`, etc.), it was missing a specific case for `PropertyKey::StringLiteral` and was falling through to the default case that calls `key.to_expression().print_expr()`, which allows backticks by default. ## Solution Added explicit handling for `PropertyKey::StringLiteral` in the `AssignmentTargetPropertyProperty::r#gen` method to call `p.print_string_literal(s, /* allow_backtick */ false)`, ensuring consistency with how property keys are handled elsewhere in the codebase. The fix: - Only affects destructuring assignments with string literal property keys - Maintains existing behavior for all other cases (identifiers, computed properties, etc.) - Follows the same pattern used in other `PropertyKey` implementations throughout the codebase ## Testing Added comprehensive test cases covering: - Basic hyphenated property keys: `{"aria-label": ariaLabel}` - Properties with newlines: `{"key-with-newline\n": val}` - Mixed computed and literal properties: `{["computed"]: a, "literal": b}` - Destructuring declarations: `let {"test-key": testKey} = obj` - Object literals with string keys: `({ "test-key": key })` - Class properties with string keys: `(class { "test-key" = key })` All existing tests continue to pass, confirming the fix doesn't break any existing functionality. <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click [here](https://survey3.medallia.com/?EAHeSx-AP01bZqG0Ld9QLQ) to start the survey.
b285299 to
bf50a02
Compare
Fixes #13558
Problem
When minifying JavaScript code with destructuring assignments that contain string literal property keys (especially those with hyphens), the codegen was incorrectly using backticks instead of double quotes:
Root Cause
The issue was in the
AssignmentTargetPropertyPropertyimplementation incrates/oxc_codegen/src/gen.rs. Unlike the regularPropertyKeyimplementation and other similar constructs (TSMethodSignature,TSPropertySignature, etc.), it was missing a specific case forPropertyKey::StringLiteraland was falling through to the default case that callskey.to_expression().print_expr(), which allows backticks by default.Solution
Added explicit handling for
PropertyKey::StringLiteralin theAssignmentTargetPropertyProperty::r#genmethod to callp.print_string_literal(s, /* allow_backtick */ false), ensuring consistency with how property keys are handled elsewhere in the codebase.The fix:
PropertyKeyimplementations throughout the codebaseTesting
Added comprehensive test cases covering:
{"aria-label": ariaLabel}{"key-with-newline\n": val}{["computed"]: a, "literal": b}let {"test-key": testKey} = obj({ "test-key": key })(class { "test-key" = key })All existing tests continue to pass, confirming the fix doesn't break any existing functionality.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.