Skip to content
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

Add a new option to control element comparison #1120

Closed
wants to merge 5 commits into from

Conversation

theKashey
Copy link

@theKashey theKashey commented May 25, 2018

Just adding an option (to pair vnode) to hook element comparison. Required by #1007
Another option is to use @arqex's code - master...arqex:master with setComponentComparator function. It is just a matter of taste.

Looking forward to make Preact quite 🔥[loadable]
(😕I did not found tests for vnode, and did not add my ones next to them 😭)

@coveralls
Copy link

coveralls commented May 25, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 06d2463 on theKashey:master into 7c3e6fe on developit:master.

@theKashey
Copy link
Author

New version of React-Hot-Loader with partial Preact support was released.
Waiting for this stuff for full compliance.

* @private
*/
export function areComponentsEqual(component1, component2) {
return options.areComponentsEqual
Copy link
Member

Choose a reason for hiding this comment

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

One could remove the if check here by making it the default implementation for options.areComponentsEqual

Copy link
Author

Choose a reason for hiding this comment

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

Pro - speed matters
Cons - you will be unable to revert configuration. Not a big deal, but could be nasty.

Copy link
Author

Choose a reason for hiding this comment

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

Probably one if here is way faster than one more function call you are asking for.

Copy link
Author

Choose a reason for hiding this comment

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

@marvinhagemeister - you were right.
My variant is 4x times slower than yours, and yours are equal to the base line.

https://jsperf.com/call-or-if/1

@theKashey
Copy link
Author

Obsolete

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

Successfully merging this pull request may close these issues.

3 participants