-
Notifications
You must be signed in to change notification settings - Fork 425
Refactor of createElement props/config to fix bugs + use snapshotting #2070
Refactor of createElement props/config to fix bugs + use snapshotting #2070
Conversation
src/evaluators/JSXElement.js
Outdated
// i.e. <div {...something} /> --> React.createElement("div", something) | ||
// if (abstractSpreadCount === 1 && astAttributes.length === 1) { | ||
// return spreadValue; | ||
// } |
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.
Commented out by mistake?
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.
Yep, I was testing to make sure it worked with and without, but then forgot to enable it again :P
src/evaluators/JSXElement.js
Outdated
flagPropsWithNoPartialKeyOrRef(realm, config); | ||
} | ||
} | ||
applyObjectAssignConfigsFoReactElement(realm, config, abstractPropsArgs); |
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.
typo
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.
Doh :D
src/react/elements.js
Outdated
} | ||
} | ||
} | ||
// if defaultPropsEvauated === the amount of properties defaultProps has, then we've successfully |
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.
spelling of defaultPropsEvauated
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.
I couldn't find anything obviously wrong.
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.
@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Release notes: fixes a range of spread bugs with ReactElements
This is a very important PR for React reconciliation, it fixes many undiscovered bugs and adds a huge amount of test coverage that was previously missing.
Whilst testing quite complex cases of JSX spreads in combination with defaultProps on our internal bundle I noticed that there were some bugs appearing, but because the branches where these bugs were appearing were not used on firstRender, it meant we got away with it on our internal tests.
We now use snapshotting and properly evaluateForEffects when recovering from
Object.assign
with ReactElement creation of config/props. We also properly use thetemporalAlias
to ensure we reference the correct object.