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

Bypass props during hydration #1697

Merged
merged 4 commits into from
Jul 7, 2019

Conversation

developit
Copy link
Member

This fixes a couple things that end up being best done together:

  • instead of special-casing the multiple prop by applying it before diffing other props, it leverages the fact that preact already special-cases the value and checked props. Pulling these out into diffElementNodes() ensures they are applied last.

  • during hydration (excessDomChildren==null), never call diffProps(). This makes it so that Preact no longer diffs attributes and props at all during hydration - previously only unknown attributes were ignored, but for performance reasons and to match other VDOM libraries it's worth also not diffing known attributes

  • don't diff children when a node has dangerouslySetInnerHTML applied. This avoids the case where you have a node with both dangerouslySetInnerHTML and children specified, which previously would continually remove children. The solution in this PR probably doesn't correctly unmount children when transitioning from JSX children to dangerouslySetInnerHTML. This could perhaps be fixed by having the existence of dangerouslySetInnerHTML for a VNode assign vnode._children=[].

  • styles: this should be a bit of a performance optimization for styles. It's hard to verify, but I think I'm seeing some relatively consistent numbers in the perf tests showing this to be the case. I'd originally made the change in order to fix the style issue someone reported this week (can't find an open issue about it for some reason)

@coveralls
Copy link

coveralls commented Jun 9, 2019

Coverage Status

Coverage increased (+0.5%) to 99.468% when pulling 317d49a on bypass-props-during-hydration into 40eb4a4 on master.

src/diff/index.js Outdated Show resolved Hide resolved
src/diff/index.js Outdated Show resolved Hide resolved
@marvinhagemeister
Copy link
Member

Love the direction this is going! This way of special casing some attributes looks a lot cleaner 👍

The first test failure seems to be a test case that needs to be updated for the changes in this PR. The other one may likely be only related to a different ordering in the assertions. I vaguely remember having swapped the attributes assertion order last time I touched diffProps 🍀

@marvinhagemeister
Copy link
Member

Played a bit with the changes in this PR and it looks like event handlers won't be attached anymore on hydrate. Made a new test case for this and it works on master, but fails in this branch.

it('should attach event handlers on hydrate', () => {
  let spy = sinon.spy();
  scratch.innerHTML = '<span>Test</span>';
  let vnode = <span onClick={spy}>Test</span>;

  hydrate(vnode, scratch);

  scratch.firstChild.click();
  expect(spy).to.be.calledOnce;
});

@marvinhagemeister
Copy link
Member

FYI: Rebased it against master and added the failing test from the comments.

@JoviDeCroock
Copy link
Member

Added this to project board since it fixes #1719

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Jun 21, 2019

@developit I'd love if you can take another look at the recent changes. To be able to attach event listeners during hydration we need to call diffProps. Because oldProps is always an empty object during hydration the first loop will do nothing. For the second loop where the properties are created, I added a condition to check if we're in hydration mode or not.

Note: Rebased the branch against the recent changes in master and force-pushed it.

Copy link
Member

@cristianbote cristianbote left a comment

Choose a reason for hiding this comment

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

Awesome stuff! 😄
Looks like this adds +7B? https://travis-ci.org/preactjs/preact/builds/549021011#L1692

if (i!=='children' && i!=='key' && !(i in newProps)) {
setProperty(dom, i, null, oldProps[i], isSvg);
for (i in newProps) {
if ((!hydrate || typeof newProps[i]=='function') && i!=='value' && i!=='checked' && oldProps[i]!==newProps[i]) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@marvinhagemeister idea: check for i[0]=='o' && i[1]=='n' instead? it should be ~free.

Copy link
Member

@marvinhagemeister marvinhagemeister Jul 7, 2019

Choose a reason for hiding this comment

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

Tried that but it's +8B bigger

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Jul 7, 2019

Rebased and the byte size is only up by +3B 🎉

@marvinhagemeister marvinhagemeister merged commit 9d46573 into master Jul 7, 2019
@marvinhagemeister marvinhagemeister deleted the bypass-props-during-hydration branch July 7, 2019 10:52
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