From 04c1d22a323c679a1bfb923b0a2cb7aa01dd0b3f Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Fri, 17 Jan 2020 11:40:05 -0800 Subject: [PATCH 01/10] assembilg all of @developit's work --- mangle.json | 2 ++ src/create-element.js | 2 ++ src/diff/children.js | 5 +++++ src/diff/index.js | 22 ++++++++++++++++++++++ 4 files changed, 31 insertions(+) diff --git a/mangle.json b/mangle.json index 221e76e3e2..2ee49e2aa6 100644 --- a/mangle.json +++ b/mangle.json @@ -27,6 +27,8 @@ "$_children": "__k", "$_suspensions": "__u", "$_dom": "__e", + "$_hydrateDom": "__s", + "$_hydrating": "__h", "$_component": "__c", "$__html": "__html", "$_parent": "__", diff --git a/src/create-element.js b/src/create-element.js index 45f62bd5cb..e7ec9b8385 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -70,6 +70,8 @@ export function createVNode(type, props, key, ref) { _dom: null, _lastDomChild: null, _component: null, + _hydrateDom: null, + _hydrating: false, constructor: undefined }; diff --git a/src/diff/children.js b/src/diff/children.js index 21f3366f23..8094f0dd7c 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -123,6 +123,11 @@ export function diffChildren( firstChildDom = newDom; } + if (excessDomChildren != null) { + const index = excessDomChildren.indexOf(newDom); + if (index !== -1) excessDomChildren[index] = null; + } + if (childVNode._lastDomChild != null) { // Only Fragments or components that return Fragment like VNodes will // have a non-null _lastDomChild. Continue the diff from the end of diff --git a/src/diff/index.js b/src/diff/index.js index 338c73bf1a..c0189a5952 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -40,6 +40,16 @@ export function diff( // constructor as undefined. This to prevent JSON-injection. if (newVNode.constructor !== undefined) return null; + // If this was the innermost VNode at a point where the tree suspended, + // pick up diffing where we left off using the saved DOM element and hydration state. + if (oldVNode._hydrateDom && excessDomChildren == null) { + oldDom = oldVNode._hydrateDom; + excessDomChildren = [oldDom]; + newVNode._dom = oldDom; + oldVNode._hydrateDom = null; + isHydrating = oldVNode._hydrating; + } + if ((tmp = options._diff)) tmp(newVNode); try { @@ -215,6 +225,13 @@ export function diff( if ((tmp = options.diffed)) tmp(newVNode); } catch (e) { + if (isHydrating) { + // Before bailing out, mark the current VNode with the DOM element and hydration state. + // We can use this information if we return here to render later on. + newVNode._dom = oldDom; + oldVNode._hydrateDom = oldDom; + oldVNode._hydrating = true; + } options._catchError(e, newVNode, oldVNode); } @@ -291,6 +308,11 @@ function diffElementNodes( } if (dom == null) { + if (excessDomChildren != null) { + const index = excessDomChildren.indexOf(dom); + if (index !== -1) excessDomChildren[index] = null; + } + if (newVNode.type === null) { return document.createTextNode(newProps); } From dab21aa05ae0632217d8d09f9666caaf23c29b1a Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Fri, 17 Jan 2020 11:45:54 -0800 Subject: [PATCH 02/10] addressing comments --- src/diff/index.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index c0189a5952..ddba0064ee 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -43,9 +43,8 @@ export function diff( // If this was the innermost VNode at a point where the tree suspended, // pick up diffing where we left off using the saved DOM element and hydration state. if (oldVNode._hydrateDom && excessDomChildren == null) { - oldDom = oldVNode._hydrateDom; + newVNode._dom = oldDom = oldVNode._hydrateDom; excessDomChildren = [oldDom]; - newVNode._dom = oldDom; oldVNode._hydrateDom = null; isHydrating = oldVNode._hydrating; } @@ -228,8 +227,7 @@ export function diff( if (isHydrating) { // Before bailing out, mark the current VNode with the DOM element and hydration state. // We can use this information if we return here to render later on. - newVNode._dom = oldDom; - oldVNode._hydrateDom = oldDom; + oldVNode._hydrateDom = newVNode._dom = oldDom; oldVNode._hydrating = true; } options._catchError(e, newVNode, oldVNode); From 422bc72185d6cd488f39afe5486dd5e9e2a4845a Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Fri, 17 Jan 2020 11:55:05 -0800 Subject: [PATCH 03/10] adding test changes --- src/diff/children.js | 3 +-- test/browser/render.test.js | 10 +++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index 8094f0dd7c..bc8c9ca4af 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -124,8 +124,7 @@ export function diffChildren( } if (excessDomChildren != null) { - const index = excessDomChildren.indexOf(newDom); - if (index !== -1) excessDomChildren[index] = null; + excessDomChildren[excessDomChildren.indexOf(newDom)] = null; } if (childVNode._lastDomChild != null) { diff --git a/test/browser/render.test.js b/test/browser/render.test.js index f1d501cd1d..7ea9d7eefd 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -1271,21 +1271,25 @@ describe('render()', () => { scratch.appendChild(container); render(
Hello
, scratch, scratch.firstElementChild); expect(scratch.innerHTML).to.equal('
Hello
'); + const child = scratch.firstElementChild; render(
Hello
, scratch, scratch.firstElementChild); expect(scratch.innerHTML).to.equal('
Hello
'); + expect(scratch.firstElementChild).to.equal(child); }); it('should replaceNode after rendering', () => { function App({ i }) { return

{i}

; } - + scratch.innerHTML = ''; render(, scratch); expect(scratch.innerHTML).to.equal('

2

'); + const child = scratch.firstElementChild; - render(, scratch, scratch.firstChild); + render(, scratch, child); expect(scratch.innerHTML).to.equal('

3

'); + expect(scratch.firstElementChild).to.equal(child); }); it('should not cause infinite loop with referentially equal props', () => { @@ -1325,7 +1329,7 @@ describe('render()', () => { it('should use replaceNode as render root and not inject into it', () => { const childA = scratch.querySelector('#a'); render(
contents
, scratch, childA); - expect(scratch.querySelector('#a')).to.equalNode(childA); + expect(scratch.querySelector('#a')).to.equal(childA); expect(childA.innerHTML).to.equal('contents'); }); From 3ecbb031f2111ed9aa7539b4a9937177f652265d Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Fri, 17 Jan 2020 13:39:08 -0800 Subject: [PATCH 04/10] addressing comments --- src/diff/index.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index 5d021324d8..8e0302efb8 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -307,8 +307,7 @@ function diffElementNodes( if (dom == null) { if (excessDomChildren != null) { - const index = excessDomChildren.indexOf(dom); - if (index !== -1) excessDomChildren[index] = null; + excessDomChildren[excessDomChildren.indexOf(dom)] = null; } if (newVNode.type === null) { From fec4986715fb1eddb5ef1e41fab0041b91faf497 Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Sat, 18 Jan 2020 13:45:38 -0800 Subject: [PATCH 05/10] adding suspense hydration tests --- test/browser/hydrate.test.js | 46 ++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/test/browser/hydrate.test.js b/test/browser/hydrate.test.js index 380416d89d..d4f661ec6c 100644 --- a/test/browser/hydrate.test.js +++ b/test/browser/hydrate.test.js @@ -1,4 +1,6 @@ import { createElement, hydrate, Fragment } from 'preact'; +import { Suspense } from 'preact/compat'; +import { useState } from 'preact/hooks'; import { setupScratch, teardown, @@ -7,11 +9,13 @@ import { } from '../_util/helpers'; import { ul, li, div } from '../_util/dom'; import { logCall, clearLog, getLog } from '../_util/logCall'; +import { setupRerender } from 'preact/test-utils'; /** @jsx createElement */ describe('hydrate()', () => { let scratch; + let rerender; const List = ({ children }) =>
    {children}
; const ListItem = ({ children }) =>
  • {children}
    • ; @@ -27,6 +31,7 @@ describe('hydrate()', () => { beforeEach(() => { scratch = setupScratch(); + rerender = setupRerender(); }); afterEach(() => { @@ -276,4 +281,45 @@ describe('hydrate()', () => { hydrate(, element); expect(element.innerHTML).to.equal('

    hello bar

    '); }); + + it('should reuse suspense component markup when suspense resolves during hydration', () => { + scratch.innerHTML = '

    hello bar

    Hello foo

    '; + const element = scratch; + let resolver; + const Component = () => { + const [state, setState] = useState(false); + if (!state) { + throw new Promise(resolve => { + resolver = () => { + setState(true); + resolve(); + }; + }); + } else { + return

    hello bar

    ; + } + }; + const App = () => { + return ( +
    + + + +

    Hello foo

    +
    + ); + }; + hydrate(, element); + rerender(); + expect(element.innerHTML).to.equal( + '

    hello bar

    Hello foo

    ' + ); + const removeChildSpy = sinon.spy(element.firstChild, 'removeChild'); + resolver(); + rerender(); + expect(removeChildSpy).to.be.not.called; + expect(element.innerHTML).to.equal( + '

    hello bar

    Hello foo

    ' + ); + }); }); From 81291f67d1356a5e63206c0cf93cc3e12e84e3d4 Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Tue, 21 Jan 2020 13:55:36 -0800 Subject: [PATCH 06/10] fixing hydration fallback bug --- compat/src/suspense.js | 20 +++++++++++++++++--- test/browser/hydrate.test.js | 2 +- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/compat/src/suspense.js b/compat/src/suspense.js index 9051d075f2..700508bc3d 100644 --- a/compat/src/suspense.js +++ b/compat/src/suspense.js @@ -11,7 +11,11 @@ options._catchError = function(error, newVNode, oldVNode) { for (; (vnode = vnode._parent); ) { if ((component = vnode._component) && component._childDidSuspend) { // Don't call oldCatchError if we found a Suspense - return component._childDidSuspend(error, newVNode._component); + return component._childDidSuspend( + error, + newVNode._component, + oldVNode && oldVNode._hydrateDom && oldVNode._hydrating + ); } } } @@ -42,8 +46,13 @@ Suspense.prototype = new Component(); /** * @param {Promise} promise The thrown promise * @param {Component} suspendingComponent The suspending component + * @param {Boolean} isSuspendedDuringHydration is Suspension done during hydration */ -Suspense.prototype._childDidSuspend = function(promise, suspendingComponent) { +Suspense.prototype._childDidSuspend = function( + promise, + suspendingComponent, + isSuspendedDuringHydration +) { /** @type {import('./internal').SuspenseComponent} */ const c = this; @@ -79,7 +88,12 @@ Suspense.prototype._childDidSuspend = function(promise, suspendingComponent) { } }; - if (!c._suspensions++) { + /** + * We do not set `suspended: true` during hydration because we want the actual markup to remain on screen + * and hydrate it when the suspense actually gets resolved. + * While in non-hydration cases the usual fallbac -> component flow would occour. + */ + if (!isSuspendedDuringHydration && !c._suspensions++) { c.setState({ _suspended: (c._detachOnNextRender = c._vnode._children[0]) }); } promise.then(onResolved, onResolved); diff --git a/test/browser/hydrate.test.js b/test/browser/hydrate.test.js index d4f661ec6c..8dbda97b45 100644 --- a/test/browser/hydrate.test.js +++ b/test/browser/hydrate.test.js @@ -302,7 +302,7 @@ describe('hydrate()', () => { const App = () => { return (
    - + baz
    }>

    Hello foo

    From 74e7934dbb63781cb8b92b261b6875dbc7884b87 Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Wed, 22 Jan 2020 08:59:33 -0800 Subject: [PATCH 07/10] removing _hydrating flag --- compat/src/suspense.js | 2 +- mangle.json | 1 - src/create-element.js | 1 - src/diff/index.js | 2 -- 4 files changed, 1 insertion(+), 5 deletions(-) diff --git a/compat/src/suspense.js b/compat/src/suspense.js index 700508bc3d..a4b72f67f8 100644 --- a/compat/src/suspense.js +++ b/compat/src/suspense.js @@ -14,7 +14,7 @@ options._catchError = function(error, newVNode, oldVNode) { return component._childDidSuspend( error, newVNode._component, - oldVNode && oldVNode._hydrateDom && oldVNode._hydrating + oldVNode && oldVNode._hydrateDom ); } } diff --git a/mangle.json b/mangle.json index 2ee49e2aa6..c413f2d2c6 100644 --- a/mangle.json +++ b/mangle.json @@ -28,7 +28,6 @@ "$_suspensions": "__u", "$_dom": "__e", "$_hydrateDom": "__s", - "$_hydrating": "__h", "$_component": "__c", "$__html": "__html", "$_parent": "__", diff --git a/src/create-element.js b/src/create-element.js index e7ec9b8385..a1b74ae6d8 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -71,7 +71,6 @@ export function createVNode(type, props, key, ref) { _lastDomChild: null, _component: null, _hydrateDom: null, - _hydrating: false, constructor: undefined }; diff --git a/src/diff/index.js b/src/diff/index.js index 8e0302efb8..69d8671ae5 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -46,7 +46,6 @@ export function diff( newVNode._dom = oldDom = oldVNode._hydrateDom; excessDomChildren = [oldDom]; oldVNode._hydrateDom = null; - isHydrating = oldVNode._hydrating; } if ((tmp = options._diff)) tmp(newVNode); @@ -228,7 +227,6 @@ export function diff( // Before bailing out, mark the current VNode with the DOM element and hydration state. // We can use this information if we return here to render later on. oldVNode._hydrateDom = newVNode._dom = oldDom; - oldVNode._hydrating = true; } options._catchError(e, newVNode, oldVNode); } From 573dbbd3f8c5539b50a30092ab793c6f4ee70651 Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Wed, 22 Jan 2020 09:07:29 -0800 Subject: [PATCH 08/10] passing vnode instead of boolean --- compat/src/internal.d.ts | 3 ++- compat/src/suspense.js | 12 ++++-------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/compat/src/internal.d.ts b/compat/src/internal.d.ts index 822684c990..f5c084ad36 100644 --- a/compat/src/internal.d.ts +++ b/compat/src/internal.d.ts @@ -15,7 +15,8 @@ export interface Component

    extends PreactComponent { _childDidSuspend?( error: Promise, - suspendingComponent: Component + suspendingComponent: Component, + oldVNode?: VNode ): void; _suspendedComponentWillUnmount?(): void; } diff --git a/compat/src/suspense.js b/compat/src/suspense.js index a4b72f67f8..d84d35ca16 100644 --- a/compat/src/suspense.js +++ b/compat/src/suspense.js @@ -11,11 +11,7 @@ options._catchError = function(error, newVNode, oldVNode) { for (; (vnode = vnode._parent); ) { if ((component = vnode._component) && component._childDidSuspend) { // Don't call oldCatchError if we found a Suspense - return component._childDidSuspend( - error, - newVNode._component, - oldVNode && oldVNode._hydrateDom - ); + return component._childDidSuspend(error, newVNode._component, oldVNode); } } } @@ -46,12 +42,12 @@ Suspense.prototype = new Component(); /** * @param {Promise} promise The thrown promise * @param {Component} suspendingComponent The suspending component - * @param {Boolean} isSuspendedDuringHydration is Suspension done during hydration + * @param {import('./internal').VNode} oldVNode old VNode caught in the _catchError options */ Suspense.prototype._childDidSuspend = function( promise, suspendingComponent, - isSuspendedDuringHydration + oldVNode ) { /** @type {import('./internal').SuspenseComponent} */ const c = this; @@ -93,7 +89,7 @@ Suspense.prototype._childDidSuspend = function( * and hydrate it when the suspense actually gets resolved. * While in non-hydration cases the usual fallbac -> component flow would occour. */ - if (!isSuspendedDuringHydration && !c._suspensions++) { + if (!(oldVNode && oldVNode._hydrateDom) && !c._suspensions++) { c.setState({ _suspended: (c._detachOnNextRender = c._vnode._children[0]) }); } promise.then(onResolved, onResolved); From a0833e4057a413ac1bb198619e8ce1d205e34a7e Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Wed, 22 Jan 2020 09:09:37 -0800 Subject: [PATCH 09/10] fixing jsdocs --- compat/src/suspense.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/src/suspense.js b/compat/src/suspense.js index d84d35ca16..5cf53a7411 100644 --- a/compat/src/suspense.js +++ b/compat/src/suspense.js @@ -42,7 +42,7 @@ Suspense.prototype = new Component(); /** * @param {Promise} promise The thrown promise * @param {Component} suspendingComponent The suspending component - * @param {import('./internal').VNode} oldVNode old VNode caught in the _catchError options + * @param {import('./internal').VNode | null | undefined} oldVNode old VNode caught in the _catchError options */ Suspense.prototype._childDidSuspend = function( promise, From 5ba7ef2bc2d849892745d8b9b074e07b4624f01e Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Fri, 24 Jan 2020 08:45:44 -0800 Subject: [PATCH 10/10] taking the common condition out --- src/diff/index.js | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index c6b3b2c58c..23bf8b92b7 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -303,11 +303,12 @@ function diffElementNodes( } } - if (dom == null) { - if (excessDomChildren != null) { - excessDomChildren[excessDomChildren.indexOf(dom)] = null; - } + if (excessDomChildren != null && dom) { + excessDomChildren[excessDomChildren.indexOf(dom)] = null; + excessDomChildren = EMPTY_ARR.slice.call(dom.childNodes); + } + if (dom == null) { if (newVNode.type === null) { return document.createTextNode(newProps); } @@ -323,19 +324,10 @@ function diffElementNodes( } if (newVNode.type === null) { - if (excessDomChildren != null) { - excessDomChildren[excessDomChildren.indexOf(dom)] = null; - } - if (oldProps !== newProps && dom.data != newProps) { dom.data = newProps; } } else if (newVNode !== oldVNode) { - if (excessDomChildren != null) { - excessDomChildren[excessDomChildren.indexOf(dom)] = null; - excessDomChildren = EMPTY_ARR.slice.call(dom.childNodes); - } - oldProps = oldVNode.props || EMPTY_OBJ; let oldHtml = oldProps.dangerouslySetInnerHTML;