-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Optimize createElement() #2135
Optimize createElement() #2135
Conversation
props = assign({}, props); | ||
let normalizedProps = {}, | ||
i; | ||
for (i in props) { |
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 clever
src/create-element.js
Outdated
// Note: type may be undefined in development, must never error here. | ||
if (typeof type === 'function' && type.defaultProps != null) { | ||
for (i in type.defaultProps) { | ||
if (typeof normalizedProps[i] === 'undefined') |
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't we just do === undefined or is typeof cheaper?
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 believe in this case we can use === yes, I actually was testing this and forgot to remove it.
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.
🎉
test/shared/createElement.test.js
Outdated
expect(<div />).to.have.property('key').that.is.undefined; | ||
expect(<div a="a" />).to.have.property('key').that.is.undefined; | ||
expect(<div />).to.have.property('key').that.is.empty; | ||
expect(<div a="a" />).to.have.property('key').that.is.empty; |
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 think these assertions want to be .to.not.exist
which would pass for null
or undefined
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.
whoops, I literally read that in the docs and then wrote .empty
. Tired me is tired!
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.
So good 🙌 ❤️
Avoiding shape mutation on props seems to give a 20% improvement in benchmarks.