-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
[compiler] Wrap inline jsx transform codegen in conditional #31267
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@mofeiZ it looks like the |
Ahh looks like the issue is that the newly added test fixture writes to Do you need to set the global |
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.
Sorry for the delay on review, see comments
...currentBlock, | ||
id: fallthroughBlockId, | ||
instructions: fallthroughBlockInstructions, |
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.
probably safer to reset phis, preds, etc here, or more generally to just copy the specific properties you want to keep (which is really only block.kind)
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.
Updated to do explicit copies. I think we want to reuse the terminal from the current block in addition to the kind since we are splitting here?
const varInstruction: Instruction = { | ||
id: makeInstructionId(0), | ||
lvalue: {...varLValuePlace}, | ||
value: { | ||
kind: 'DeclareLocal', | ||
lvalue: {place: {...varPlace}, kind: InstructionKind.Let}, | ||
type: null, | ||
loc: instr.value.loc, | ||
}, | ||
loc: instr.loc, | ||
}; | ||
currentBlockInstructions.push(varInstruction); |
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.
Hmm this isn't safe. There are two main categories of blocks which correspond roughly to statement-level blocks (kind 'block') and expression-level blocks (kind 'logical' etc). It's only safe to add a variable declaration at a block statement. So i think what you'd need to do is write this as a ternary, with an unnamed (non-promoted) variable that is getting assigned one of two different values without a variable declaration.
ie you're doing:
let t0;
if (__DEV__) {
t0 = { $$typeof: 'react.element', ...}
} else {
t0 = jsx(...);
}
When it needs to be
__DEV__ ? { $$typeof: ... } : jsx(...)
So that it works in statement and expression contexts.
Ternaries are a bit tedious to create, you need both a ternary terminal and a branch terminal. But you get to avoid the variable declaration. You'll still need to create the phi in the fallthrough block.
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.
Updated to skip value blocks with a TODO test and a diagnostic log
A failing example would be something like: function Component({cond}) {
return cond ? <div /> : null;
} because we'll try to create a variable declaration inside the consequent of the ternary, but that's a statement where only expressions are allowed. |
Chatted with @mofeiZ about this briefly — she suggested that we could unblock getting some experimental results by just skipping JSX within value blocks. That seems like a great compromise, and if the data is promising we can come back to handle value blocks. |
8260ae0
to
c043c1e
Compare
c043c1e
to
8bb3ca3
Compare
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.
awesome!
JSX inlining is a prod-only optimization. We want to enforce this while maintaining the same compiler output in DEV and PROD.
Here we add a conditional to the transform that only replaces JSX with object literals outside of DEV. Then a later build step can handle DCE based on the value of
__DEV__