-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
allowMutablePropsOnTags
: cache JSX constant elements with function props
#12975
allowMutablePropsOnTags
: cache JSX constant elements with function props
#12975
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4f9b9c4:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51305/ |
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.
Can we get a few test cases for nested do expressions?
I think this series of PRs is really clever workaround for the hoisting issue. Thanks!
// If we allow mutable props, tags with function expressions can be | ||
// safely hoisted. | ||
const { mutablePropsAllowed } = state; | ||
if (mutablePropsAllowed && path.isFunction()) return skip(); |
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'd propose we traverse with the targetScopeVisitor
here, so we could keep the two traversals together.
4f9b9c4
to
425f9f7
Compare
if (path.isIdentifier()) { | ||
const binding = path.scope.getBinding(path.node.name); | ||
if (binding && binding.constant) return; | ||
} | ||
|
||
if (!path.isImmutable()) { | ||
// If it's not immutable, it may still be a pure expression, such as string concatenation. |
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 was not actually changed; I suggest hiding whitespace changes when reviewing.
allowMutablePropsOnTags
allowMutablePropsOnTags
: cache JSX const elements w/ fn props
allowMutablePropsOnTags
: cache JSX const elements w/ fn propsallowMutablePropsOnTags
: cache JSX constant elements with function props
@jridgewell You were right!
I had to split
immutabiltyVisitor
andtargetScopeVisitor
in two visitors because I had to skip functions when checking for immutability, but traverse them when collecting references.