Skip to content

Optimize createElement()#2135

Merged
marvinhagemeister merged 5 commits into
masterfrom
optimize-createelement
Nov 18, 2019
Merged

Optimize createElement()#2135
marvinhagemeister merged 5 commits into
masterfrom
optimize-createelement

Conversation

@developit

Copy link
Copy Markdown
Member

Avoiding shape mutation on props seems to give a 20% improvement in benchmarks.

Comment thread src/create-element.js
props = assign({}, props);
let normalizedProps = {},
i;
for (i in props) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clever

Comment thread 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')

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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.

@andrewiggins andrewiggins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Comment thread 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;

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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!

@marvinhagemeister marvinhagemeister left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So good 🙌 ❤️

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 19e73bb on optimize-createelement into 9cf8cc7 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants