-
Notifications
You must be signed in to change notification settings - Fork 47.4k
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] Support enableRefAsProp in jsx transform #31558
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Makes sense! One comment to address before landing though
case 'ref': { | ||
refProperty = { | ||
kind: 'ObjectProperty', | ||
key: {name: 'ref', kind: 'string'}, | ||
type: 'property', | ||
place: {...prop.place}, | ||
}; |
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.
reusing the object property means reusing the Place instances, which is invalid. let's remove the fallthrough and just assign to both refProperty
and push to props
with distinct object property values and places
…31726) When supporting ref as prop in #31558, I missed fixing the optimization to pass a spread-props-only props object in without an additional object copy. In the case that we have only a ref along with a spread, we cannot return only the spread object. This results in dropping the ref. In this example ```javascript <Foo ref={ref} {...props} /> ``` The bugged output is: ```javascript { // ... props: props } ``` With this change we now get the correct output: ```javascript { // ... props: {ref: ref, ...props} } ```
Since
enableRefAsProp
shipped everywhere, the ReactElement implementation on prod puts refs on bothelement.ref
andelement.props.ref
. Here we let theref
case fall through so its now available on props, matching the JSX runtime.