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 replaceNode switch #141

Merged
merged 3 commits into from
Mar 20, 2018
Merged

Conversation

BehindTheMath
Copy link
Collaborator

This adds a new default switch which is functionally the same as the outerHTML switch, but uses Node.replaceChild() instead of outerHTML, so it doesn't require switchElementsAlt.

@BehindTheMath
Copy link
Collaborator Author

@robinnorth Should we consider using this as the default switch instead of outerHTML?

@BehindTheMath BehindTheMath added this to the 0.2.6 milestone Mar 16, 2018
@robinnorth
Copy link
Collaborator

Would be good to know if there's any performance difference between the two switches (and any browser-specific known issues that need considering, not that I can think of any) before making it the new default. Making it the new default would also be a breaking change, so you'd have to do that in 0.3.0 (still think that should be 1.0.0!).

@BehindTheMath
Copy link
Collaborator Author

Would be good to know if there's any performance difference between the two switches

Based on this jsperf test, replaceChild is 3x as fast as innerHTML. Regardless, I don't think the practical difference will be more than a few milliseconds, since I doubt anyone is replacing more than a few elements.

(and any browser-specific known issues that need considering, not that I can think of any)

According to this (referenced by Caniuse), parentNode and replaceChild are supported properly by all browsers.

Making it the new default would also be a breaking change, so you'd have to do that in 0.3.0 (still think that should be 1.0.0!).

Absolutely. I would make a new PR for that after v0.2.6 is finished.

@BehindTheMath BehindTheMath force-pushed the feature/add-replace-node-switch branch from 8527acb to d615b79 Compare March 18, 2018 04:20
@BehindTheMath
Copy link
Collaborator Author

I rebased on master and force-pushed to be able to update the Typescript definitions.

@robinnorth
Copy link
Collaborator

Cool, all sounds good to me.

@BehindTheMath BehindTheMath merged commit 75eb83d into master Mar 20, 2018
@BehindTheMath BehindTheMath deleted the feature/add-replace-node-switch branch March 20, 2018 14:53
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.

2 participants