From 40b246722c9085c3a144806025f56ee3c7824913 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 1 Jan 2021 20:01:14 -0800 Subject: [PATCH 1/5] Assert event listeners in hydrate tests --- test/browser/hydrate.test.js | 104 ++++++++++++++++++++++++++++++----- 1 file changed, 90 insertions(+), 14 deletions(-) diff --git a/test/browser/hydrate.test.js b/test/browser/hydrate.test.js index 1fe2062cae..94a9894a41 100644 --- a/test/browser/hydrate.test.js +++ b/test/browser/hydrate.test.js @@ -4,7 +4,8 @@ import { teardown, sortAttributes, serializeHtml, - spyOnElementAttributes + spyOnElementAttributes, + createEvent } from '../_util/helpers'; import { ul, li, div } from '../_util/dom'; import { logCall, clearLog, getLog } from '../_util/logCall'; @@ -12,10 +13,14 @@ import { logCall, clearLog, getLog } from '../_util/logCall'; /** @jsx createElement */ describe('hydrate()', () => { - let scratch, attributesSpy; + /** @type {HTMLElement} */ + let scratch; + let attributesSpy; const List = ({ children }) => ; - const ListItem = ({ children }) =>
  • {children}
    • ; + const ListItem = ({ children, onClick = null }) => ( +
  • {children}
    • + ); let resetAppendChild; let resetInsertBefore; @@ -53,6 +58,7 @@ describe('hydrate()', () => { }); it('should reuse existing DOM', () => { + const onClickSpy = sinon.spy(); const html = ul([li('1'), li('2'), li('3')]); scratch.innerHTML = html; @@ -62,16 +68,22 @@ describe('hydrate()', () => { , scratch ); expect(scratch.innerHTML).to.equal(html); expect(getLog()).to.deep.equal([]); + expect(onClickSpy).not.to.have.been.called; + + scratch.querySelector('li:last-child').dispatchEvent(createEvent('click')); + + expect(onClickSpy).to.have.been.called.calledOnce; }); it('should reuse existing DOM when given components', () => { + const onClickSpy = sinon.spy(); const html = ul([li('1'), li('2'), li('3')]); scratch.innerHTML = html; @@ -81,13 +93,47 @@ describe('hydrate()', () => { 1 2 - 3 + 3 + , + scratch + ); + + expect(scratch.innerHTML).to.equal(html); + expect(getLog()).to.deep.equal([]); + expect(onClickSpy).not.to.have.been.called; + + scratch.querySelector('li:last-child').dispatchEvent(createEvent('click')); + + expect(onClickSpy).to.have.been.called.calledOnce; + }); + + it('should properly set event handlers to existing DOM when given components', () => { + const proto = Element.prototype; + sinon.spy(proto, 'addEventListener'); + + const clickHandlers = [sinon.spy(), sinon.spy(), sinon.spy()]; + + const html = ul([li('1'), li('2'), li('3')]); + + scratch.innerHTML = html; + clearLog(); + + hydrate( + + 1 + 2 + 3 , scratch ); expect(scratch.innerHTML).to.equal(html); expect(getLog()).to.deep.equal([]); + expect(proto.addEventListener).to.have.been.calledThrice; + expect(clickHandlers[2]).not.to.have.been.called; + + scratch.querySelector('li:last-child').dispatchEvent(createEvent('click')); + expect(clickHandlers[2]).to.have.been.calledOnce; }); it('should add missing nodes to existing DOM when hydrating', () => { @@ -168,20 +214,29 @@ describe('hydrate()', () => { scratch.innerHTML = html; clearLog(); + const clickHandlers = [sinon.spy(), sinon.spy(), sinon.spy(), sinon.spy()]; + hydrate( - 1 + 1 - 2 - 3 + 2 + 3 - 4 + 4 , scratch ); expect(scratch.innerHTML).to.equal(html); expect(getLog()).to.deep.equal([]); + expect(clickHandlers[2]).not.to.have.been.called; + + scratch + .querySelector('li:nth-child(3)') + .dispatchEvent(createEvent('click')); + + expect(clickHandlers[2]).to.have.been.called.calledOnce; }); it('should correctly hydrate root Fragments', () => { @@ -193,23 +248,44 @@ describe('hydrate()', () => { scratch.innerHTML = html; clearLog(); + const clickHandlers = [ + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy() + ]; + hydrate( - 1 - 2 + 1 + 2 - 3 - 4 + 3 + 4 -
    sibling
    +
    sibling
    , scratch ); expect(scratch.innerHTML).to.equal(html); expect(getLog()).to.deep.equal([]); + expect(clickHandlers[2]).not.to.have.been.called; + + scratch + .querySelector('li:nth-child(3)') + .dispatchEvent(createEvent('click')); + + expect(clickHandlers[2]).to.have.been.calledOnce; + expect(clickHandlers[4]).not.to.have.been.called; + + scratch.querySelector('div').dispatchEvent(createEvent('click')); + + expect(clickHandlers[2]).to.have.been.calledOnce; + expect(clickHandlers[4]).to.have.been.calledOnce; }); // Failing because the following condition in diffElementNodes doesn't evaluate to true From ebe3bc3e6821c459c074ae8e55444c092ca865c3 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 1 Jan 2021 20:01:24 -0800 Subject: [PATCH 2/5] Add some failing suspense hydration tests --- .../test/browser/suspense-hydration.test.js | 115 +++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-) diff --git a/compat/test/browser/suspense-hydration.test.js b/compat/test/browser/suspense-hydration.test.js index 0e358774d8..ce5ffdc5aa 100644 --- a/compat/test/browser/suspense-hydration.test.js +++ b/compat/test/browser/suspense-hydration.test.js @@ -7,7 +7,12 @@ import React, { Suspense } from 'preact/compat'; import { logCall, getLog, clearLog } from '../../../test/_util/logCall'; -import { setupScratch, teardown } from '../../../test/_util/helpers'; +import { + createEvent, + setupScratch, + teardown +} from '../../../test/_util/helpers'; +import { ul, li } from '../../../test/_util/dom'; import { createLazy } from './suspense-utils'; /* eslint-env browser, mocha */ @@ -101,6 +106,48 @@ describe('suspense hydration', () => { }); }); + it('should properly attach event listeners when suspending while hydrating', () => { + scratch.innerHTML = '
    Hello
    World
    '; + clearLog(); + + const helloListener = sinon.spy(); + const worldListener = sinon.spy(); + + const [Lazy, resolve] = createLazy(); + hydrate( + + +
    World!
    +     , + scratch + ); + rerender(); // Flush rerender queue to mimic what preact will really do + expect(scratch.innerHTML).to.equal('
    Hello
    World!
    '); + expect(getLog()).to.deep.equal([]); + clearLog(); + + scratch.querySelector('div:last-child').dispatchEvent(createEvent('click')); + expect(worldListener, 'worldListener 1').to.have.been.calledOnce; + + return resolve(() =>
    Hello
    ).then(() => { + rerender(); + expect(scratch.innerHTML).to.equal('
    Hello
    World!
    '); + expect(getLog()).to.deep.equal([]); + + scratch + .querySelector('div:first-child') + .dispatchEvent(createEvent('click')); + expect(helloListener, 'helloListener').to.have.been.calledOnce; + + scratch + .querySelector('div:last-child') + .dispatchEvent(createEvent('click')); + expect(worldListener, 'worldListener 2').to.have.been.calledTwice; + + clearLog(); + }); + }); + it('should allow siblings to update around suspense boundary', () => { scratch.innerHTML = '
    Count: 0
    Hello
    '; clearLog(); @@ -164,6 +211,72 @@ describe('suspense hydration', () => { }); }); + it('should properly hydrate suspense with Fragment siblings', () => { + const originalHtml = ul( + [li(0), li(1), li(2), li(3), li(4), li(5)].join('') + ); + + const listeners = [ + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy() + ]; + + scratch.innerHTML = originalHtml; + clearLog(); + + const [Lazy, resolve] = createLazy(); + hydrate( + , + scratch + ); + rerender(); // Flush rerender queue to mimic what preact will really do + expect(scratch.innerHTML).to.equal(originalHtml); + expect(getLog()).to.deep.equal([]); + expect(listeners[5]).not.to.have.been.called; + + clearLog(); + scratch.querySelector('li:last-child').dispatchEvent(createEvent('click')); + expect(listeners[5]).to.have.been.calledOnce; + + return resolve(() => ( + +
  • 2
    • +
  • 3
    • +     + )).then(() => { + rerender(); + expect(scratch.innerHTML).to.equal(originalHtml); + expect(getLog()).to.deep.equal([]); + clearLog(); + + scratch + .querySelector('li:nth-child(4)') + .dispatchEvent(createEvent('click')); + expect(listeners[3]).to.have.been.calledOnce; + + scratch + .querySelector('li:last-child') + .dispatchEvent(createEvent('click')); + expect(listeners[5]).to.have.been.calledTwice; + }); + }); + // TODO: // 1. What if props change between when hydrate suspended and suspense // resolves? From d1fe0ab4c1bc81e0e430efad1acff503defb440e Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 1 Jan 2021 23:55:39 -0800 Subject: [PATCH 3/5] Fix suspended hydration event listeners --- compat/src/suspense.js | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/compat/src/suspense.js b/compat/src/suspense.js index 833e051beb..7b46716bfd 100644 --- a/compat/src/suspense.js +++ b/compat/src/suspense.js @@ -62,6 +62,7 @@ export function Suspense() { Suspense.prototype = new Component(); /** + * @this {import('./internal').SuspenseComponent} * @param {Promise} promise The thrown promise * @param {import('./internal').VNode} suspendingVNode The suspending component */ @@ -105,7 +106,12 @@ Suspense.prototype._childDidSuspend = function(promise, suspendingVNode) { const onSuspensionComplete = () => { if (!--c._pendingSuspensionCount) { - c._vnode._children[0] = removeOriginal(c.state._suspended); + // If the suspension was during hydration we don't need to restore the + // suspended children into the _children array + if (c.state._suspended) { + c._vnode._children[0] = removeOriginal(c.state._suspended); + } + c.setState({ _suspended: (c._detachOnNextRender = null) }); let suspended; @@ -121,7 +127,7 @@ Suspense.prototype._childDidSuspend = function(promise, suspendingVNode) { * While in non-hydration cases the usual fallback -> component flow would occour. */ const wasHydrating = suspendingVNode._hydrating === true; - if (!wasHydrating && !c._pendingSuspensionCount++) { + if (!c._pendingSuspensionCount++ && !wasHydrating) { c.setState({ _suspended: (c._detachOnNextRender = c._vnode._children[0]) }); } promise.then(onResolved, onResolved); @@ -131,13 +137,20 @@ Suspense.prototype.componentWillUnmount = function() { this._suspenders = []; }; +/** + * @this {import('./internal').SuspenseComponent} + * @param {import('./internal').SuspenseComponent["props"]} props + * @param {import('./internal').SuspenseState} state + */ Suspense.prototype.render = function(props, state) { if (this._detachOnNextRender) { // When the Suspense's _vnode was created by a call to createVNode // (i.e. due to a setState further up in the tree) // it's _children prop is null, in this case we "forget" about the parked vnodes to detach - if (this._vnode._children) + if (this._vnode._children) { this._vnode._children[0] = detachedClone(this._detachOnNextRender); + } + this._detachOnNextRender = null; } From 12eff66f3e2d247d1255281ec59fbd762f65936f Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 2 Jan 2021 00:04:15 -0800 Subject: [PATCH 4/5] Add suspense hydration test with Components --- .../test/browser/suspense-hydration.test.js | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/compat/test/browser/suspense-hydration.test.js b/compat/test/browser/suspense-hydration.test.js index ce5ffdc5aa..9b3bf41b7b 100644 --- a/compat/test/browser/suspense-hydration.test.js +++ b/compat/test/browser/suspense-hydration.test.js @@ -22,6 +22,11 @@ describe('suspense hydration', () => { rerender, unhandledEvents = []; + const List = ({ children }) =>
      {children}
    ; + const ListItem = ({ children, onClick = null }) => ( +
  • {children}
    • + ); + function onUnhandledRejection(event) { unhandledEvents.push(event); } @@ -277,6 +282,72 @@ describe('suspense hydration', () => { }); }); + it('should properly hydrate suspense with Component & Fragment siblings', () => { + const originalHtml = ul( + [li(0), li(1), li(2), li(3), li(4), li(5)].join('') + ); + + const listeners = [ + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy() + ]; + + scratch.innerHTML = originalHtml; + clearLog(); + + const [Lazy, resolve] = createLazy(); + hydrate( + + + 0 + 1 + + + + + + 4 + 5 + + , + scratch + ); + rerender(); // Flush rerender queue to mimic what preact will really do + expect(scratch.innerHTML).to.equal(originalHtml); + expect(getLog()).to.deep.equal([]); + expect(listeners[5]).not.to.have.been.called; + + clearLog(); + scratch.querySelector('li:last-child').dispatchEvent(createEvent('click')); + expect(listeners[5]).to.have.been.calledOnce; + + return resolve(() => ( + + 2 + 3 + + )).then(() => { + rerender(); + expect(scratch.innerHTML).to.equal(originalHtml); + expect(getLog()).to.deep.equal([]); + clearLog(); + + scratch + .querySelector('li:nth-child(4)') + .dispatchEvent(createEvent('click')); + expect(listeners[3]).to.have.been.calledOnce; + + scratch + .querySelector('li:last-child') + .dispatchEvent(createEvent('click')); + expect(listeners[5]).to.have.been.calledTwice; + }); + }); + // TODO: // 1. What if props change between when hydrate suspended and suspense // resolves? From b0c44b8f1469a8737c15a057a5afc4d3b750a198 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 2 Jan 2021 00:23:56 -0800 Subject: [PATCH 5/5] Isolate test case where lazy resolves to a Fragment --- .../test/browser/suspense-hydration.test.js | 80 ++++++++++++++++--- 1 file changed, 69 insertions(+), 11 deletions(-) diff --git a/compat/test/browser/suspense-hydration.test.js b/compat/test/browser/suspense-hydration.test.js index 9b3bf41b7b..eadb3cf96c 100644 --- a/compat/test/browser/suspense-hydration.test.js +++ b/compat/test/browser/suspense-hydration.test.js @@ -217,16 +217,13 @@ describe('suspense hydration', () => { }); it('should properly hydrate suspense with Fragment siblings', () => { - const originalHtml = ul( - [li(0), li(1), li(2), li(3), li(4), li(5)].join('') - ); + const originalHtml = ul([li(0), li(1), li(2), li(3), li(4)].join('')); const listeners = [ sinon.spy(), sinon.spy(), sinon.spy(), sinon.spy(), - sinon.spy(), sinon.spy() ]; @@ -244,8 +241,8 @@ describe('suspense hydration', () => { +
  • 3
  • 4
    • -
  • 5
    •  , scratch @@ -253,16 +250,15 @@ describe('suspense hydration', () => { rerender(); // Flush rerender queue to mimic what preact will really do expect(scratch.innerHTML).to.equal(originalHtml); expect(getLog()).to.deep.equal([]); - expect(listeners[5]).not.to.have.been.called; + expect(listeners[4]).not.to.have.been.called; clearLog(); scratch.querySelector('li:last-child').dispatchEvent(createEvent('click')); - expect(listeners[5]).to.have.been.calledOnce; + expect(listeners[4]).to.have.been.calledOnce; return resolve(() => (
  • 2
    • -
  • 3
    •  )).then(() => { rerender(); @@ -271,18 +267,80 @@ describe('suspense hydration', () => { clearLog(); scratch - .querySelector('li:nth-child(4)') + .querySelector('li:nth-child(3)') .dispatchEvent(createEvent('click')); - expect(listeners[3]).to.have.been.calledOnce; + expect(listeners[2]).to.have.been.calledOnce; scratch .querySelector('li:last-child') .dispatchEvent(createEvent('click')); - expect(listeners[5]).to.have.been.calledTwice; + expect(listeners[4]).to.have.been.calledTwice; }); }); it('should properly hydrate suspense with Component & Fragment siblings', () => { + const originalHtml = ul([li(0), li(1), li(2), li(3), li(4)].join('')); + + const listeners = [ + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy(), + sinon.spy() + ]; + + scratch.innerHTML = originalHtml; + clearLog(); + + const [Lazy, resolve] = createLazy(); + hydrate( + + + 0 + 1 + + + + + + 3 + 4 + + , + scratch + ); + rerender(); // Flush rerender queue to mimic what preact will really do + expect(scratch.innerHTML).to.equal(originalHtml); + expect(getLog()).to.deep.equal([]); + expect(listeners[4]).not.to.have.been.called; + + clearLog(); + scratch.querySelector('li:last-child').dispatchEvent(createEvent('click')); + expect(listeners[4]).to.have.been.calledOnce; + + return resolve(() => ( + + 2 + + )).then(() => { + rerender(); + expect(scratch.innerHTML).to.equal(originalHtml); + expect(getLog()).to.deep.equal([]); + clearLog(); + + scratch + .querySelector('li:nth-child(3)') + .dispatchEvent(createEvent('click')); + expect(listeners[2]).to.have.been.calledOnce; + + scratch + .querySelector('li:last-child') + .dispatchEvent(createEvent('click')); + expect(listeners[4]).to.have.been.calledTwice; + }); + }); + + it.skip('should properly hydrate suspense when resolves to a Fragment', () => { const originalHtml = ul( [li(0), li(1), li(2), li(3), li(4), li(5)].join('') );