-
Notifications
You must be signed in to change notification settings - Fork 51k
[compiler] Fix enableJsxOutlining with hyphenated JSX attribute names #36221
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
53457ac
d8622da
6e524ff
cef1721
6bbc77b
c3eac7f
9f56bc2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1963,34 +1963,35 @@ function codegenInstructionValue( | |||||||||||||||||||||||||||||||||||||
| instruction, | ||||||||||||||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||||||||||||||
| ).body; | ||||||||||||||||||||||||||||||||||||||
| const expressions = body.map(stmt => { | ||||||||||||||||||||||||||||||||||||||
| const expressions = body.flatMap(stmt => { | ||||||||||||||||||||||||||||||||||||||
| if (stmt.type === 'ExpressionStatement') { | ||||||||||||||||||||||||||||||||||||||
| return stmt.expression; | ||||||||||||||||||||||||||||||||||||||
| return [stmt.expression]; | ||||||||||||||||||||||||||||||||||||||
| } else if (t.isVariableDeclaration(stmt)) { | ||||||||||||||||||||||||||||||||||||||
| return stmt.declarations.map(declarator => { | ||||||||||||||||||||||||||||||||||||||
| if (declarator.init != null) { | ||||||||||||||||||||||||||||||||||||||
| return t.assignmentExpression( | ||||||||||||||||||||||||||||||||||||||
| '=', | ||||||||||||||||||||||||||||||||||||||
| declarator.id as t.LVal, | ||||||||||||||||||||||||||||||||||||||
| declarator.init, | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||
| return t.assignmentExpression( | ||||||||||||||||||||||||||||||||||||||
| '=', | ||||||||||||||||||||||||||||||||||||||
| declarator.id as t.LVal, | ||||||||||||||||||||||||||||||||||||||
| t.identifier('undefined'), | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can simplify
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied, thank you! Simplified to the ternary form in cef1721. |
||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1971
to
+1980
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change related to JSX outlining? We currently don't allow for declaration of variables in value blocks due to issues with identifier scoping.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is triggered by JSX outlining specifically. When The fix converts The fixture |
||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||
| if (t.isVariableDeclaration(stmt)) { | ||||||||||||||||||||||||||||||||||||||
| const declarator = stmt.declarations[0]; | ||||||||||||||||||||||||||||||||||||||
| cx.recordError( | ||||||||||||||||||||||||||||||||||||||
| new CompilerErrorDetail({ | ||||||||||||||||||||||||||||||||||||||
| reason: `(CodegenReactiveFunction::codegenInstructionValue) Cannot declare variables in a value block, tried to declare '${ | ||||||||||||||||||||||||||||||||||||||
| (declarator.id as t.Identifier).name | ||||||||||||||||||||||||||||||||||||||
| }'`, | ||||||||||||||||||||||||||||||||||||||
| category: ErrorCategory.Todo, | ||||||||||||||||||||||||||||||||||||||
| loc: declarator.loc ?? null, | ||||||||||||||||||||||||||||||||||||||
| suggestions: null, | ||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| return t.stringLiteral(`TODO handle ${declarator.id}`); | ||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||
| cx.recordError( | ||||||||||||||||||||||||||||||||||||||
| new CompilerErrorDetail({ | ||||||||||||||||||||||||||||||||||||||
| reason: `(CodegenReactiveFunction::codegenInstructionValue) Handle conversion of ${stmt.type} to expression`, | ||||||||||||||||||||||||||||||||||||||
| category: ErrorCategory.Todo, | ||||||||||||||||||||||||||||||||||||||
| loc: stmt.loc ?? null, | ||||||||||||||||||||||||||||||||||||||
| suggestions: null, | ||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| return t.stringLiteral(`TODO handle ${stmt.type}`); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| cx.recordError( | ||||||||||||||||||||||||||||||||||||||
| new CompilerErrorDetail({ | ||||||||||||||||||||||||||||||||||||||
| reason: `(CodegenReactiveFunction::codegenInstructionValue) Handle conversion of ${stmt.type} to expression`, | ||||||||||||||||||||||||||||||||||||||
| category: ErrorCategory.Todo, | ||||||||||||||||||||||||||||||||||||||
| loc: stmt.loc ?? null, | ||||||||||||||||||||||||||||||||||||||
| suggestions: null, | ||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| return [t.stringLiteral(`TODO handle ${stmt.type}`)]; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| if (expressions.length === 0) { | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
|
|
||
| ## Input | ||
|
|
||
| ```javascript | ||
| // @enableJsxOutlining | ||
| function Component() { | ||
| const [isSubmitting] = useState(false); | ||
|
|
||
| return ssoProviders.map(provider => { | ||
| return ( | ||
| <div key={provider.providerId}> | ||
| <Switch | ||
| disabled={isSubmitting} | ||
| aria-label={`Toggle ${provider.displayName}`} | ||
| /> | ||
| </div> | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| ## Code | ||
|
|
||
| ```javascript | ||
| import { c as _c } from "react/compiler-runtime"; // @enableJsxOutlining | ||
| function Component() { | ||
| const $ = _c(2); | ||
| const [isSubmitting] = useState(false); | ||
| let t0; | ||
| if ($[0] !== isSubmitting) { | ||
| t0 = ssoProviders.map((provider) => { | ||
| const T0 = _temp; | ||
| return ( | ||
| <T0 | ||
| disabled={isSubmitting} | ||
| ariaLabel={`Toggle ${provider.displayName}`} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's ok to leave here dashes
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed! The compiler now keeps hyphenated attribute names (like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But I'm pretty unsure what this is really need with dashes... Before was cleaner.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if it makes code complex you can rollback this change |
||
| key={provider.providerId} | ||
| /> | ||
| ); | ||
| }); | ||
| $[0] = isSubmitting; | ||
| $[1] = t0; | ||
| } else { | ||
| t0 = $[1]; | ||
| } | ||
| return t0; | ||
| } | ||
| function _temp(t0) { | ||
| const $ = _c(3); | ||
| const { disabled: disabled, ariaLabel: ariaLabel } = t0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unneeded rename for -const { disabled: disabled, ariaLabel: ariaLabel } = t0;
+const { disabled, 'aria-label': ariaLabel } = t0;
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed! Both issues addressed in d8622da:
This comment was marked as resolved.
Sorry, something went wrong. |
||
| let t1; | ||
| if ($[0] !== ariaLabel || $[1] !== disabled) { | ||
| t1 = ( | ||
| <div> | ||
| <Switch disabled={disabled} aria-label={ariaLabel} /> | ||
| </div> | ||
| ); | ||
| $[0] = ariaLabel; | ||
| $[1] = disabled; | ||
| $[2] = t1; | ||
| } else { | ||
| t1 = $[2]; | ||
| } | ||
| return t1; | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| ### Eval output | ||
| (kind: exception) Fixture not implemented | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // @enableJsxOutlining | ||
| function Component() { | ||
| const [isSubmitting] = useState(false); | ||
|
|
||
| return ssoProviders.map(provider => { | ||
| return ( | ||
| <div key={provider.providerId}> | ||
| <Switch | ||
| disabled={isSubmitting} | ||
| aria-label={`Toggle ${provider.displayName}`} | ||
| /> | ||
| </div> | ||
| ); | ||
| }); | ||
| } |
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.
This is a sensitive change.
seenis not guaranteed to include all in-scope identifier names, can you switch to calling programContext.newUuid() fromreact/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Imports.ts
Lines 117 to 142 in 3cb2c42
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.
Good catch on the
seenset gap. I tried two alternatives:env.programContext.newUid(baseName)— checksknownReferencedNames,scope.hasBinding,scope.hasGlobal,scope.hasReference. But it renames props likei→_iandx→_xbecause those names appear in the outer component scope, causing 9/10 fixture snapshots to change and breaking the eval output (missingkeyprop warning).env.generateGloballyUniqueIdentifierName(baseName)— usesscope.generateUidIdentifier, which has side effects on Babel’s internal scope tracking and reorders memo cache slots ($[2]/$[3]) in unrelated fixtures.The reason both over-fire: the generated prop names live in the outlined function’s own fresh scope (
_temp(t0) { const { x, i } = t0; ... }), so shadowing an outer-scopexoriis safe — there is no capture. Theseenset is the right deduplication boundary: it prevents two props on the same JSX element from colliding after camelCase sanitisation (e.g. twoaria-*attrs that both map to the same identifier). The existingenv.programContext.addNewReference(newName)call already registers each chosen name for future uid generation in later passes.Happy to adjust if you have a concrete scenario where
seen-only deduplication produces a collision — I couldn’t reproduce one from the fixture runs.