From e9a5d761185f79c3981f5ba64b091ab71a30a51a Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 25 Oct 2019 12:50:05 -0700 Subject: [PATCH 1/9] Verify hooks calls old options --- hooks/test/_util/optionsSpies.js | 18 ++++++++ hooks/test/browser/options-hooks.test.js | 57 ++++++++++++++++++++++++ 2 files changed, 75 insertions(+) create mode 100644 hooks/test/_util/optionsSpies.js create mode 100644 hooks/test/browser/options-hooks.test.js diff --git a/hooks/test/_util/optionsSpies.js b/hooks/test/_util/optionsSpies.js new file mode 100644 index 00000000000..d35d9c2cb1a --- /dev/null +++ b/hooks/test/_util/optionsSpies.js @@ -0,0 +1,18 @@ +import { options } from "preact"; + +let oldBeforeRender = options._render; +let oldAfterDiff = options.diffed; +let oldUnmount = options.unmount; + +/** @type {import('sinon').SinonSpy} */ +export const beforeRenderSpy = sinon.spy(oldBeforeRender); + +/** @type {import('sinon').SinonSpy} */ +export const afterDiffSpy = sinon.spy(oldAfterDiff); + +/** @type {import('sinon').SinonSpy} */ +export const unmountSpy = sinon.spy(oldUnmount); + +options._render = beforeRenderSpy; +options.diffed = afterDiffSpy +options.unmount = unmountSpy; diff --git a/hooks/test/browser/options-hooks.test.js b/hooks/test/browser/options-hooks.test.js new file mode 100644 index 00000000000..a5ddb9a356c --- /dev/null +++ b/hooks/test/browser/options-hooks.test.js @@ -0,0 +1,57 @@ +import { + afterDiffSpy, + beforeRenderSpy, + unmountSpy +} from "../_util/optionsSpies"; + +import { setupRerender } from "preact/test-utils"; +import { createElement, render } from "preact"; +import { setupScratch, teardown } from "../../../test/_util/helpers"; +import { useState } from "../../src"; + +/** @jsx createElement */ + +describe("hook options", () => { + /** @type {HTMLDivElement} */ + let scratch; + + /** @type {() => void} */ + let rerender; + + beforeEach(() => { + scratch = setupScratch(); + rerender = setupRerender(); + + afterDiffSpy.resetHistory(); + unmountSpy.resetHistory(); + beforeRenderSpy.resetHistory(); + }); + + afterEach(() => { + teardown(scratch); + }); + + function App() { + const [count, setCount] = useState(0); + return
{count}
; + } + + it("should call old options._render", () => { + render(, scratch); + + expect(beforeRenderSpy).to.have.been.called; + }); + + it("should call old options.diffed", () => { + render(, scratch); + + expect(afterDiffSpy).to.have.been.called; + }); + + it("should call old options.unmount", () => { + render(, scratch); + render(null, scratch); + + expect(unmountSpy).to.have.been.called; + }); +}); From 78f19486e92422e124b3c886000441c39bb3b27d Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 25 Oct 2019 14:10:21 -0700 Subject: [PATCH 2/9] Move option spy helper to general test util --- hooks/test/_util/optionsSpies.js | 18 ----------- hooks/test/browser/options-hooks.test.js | 4 +-- test/_util/optionSpies.js | 39 ++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 20 deletions(-) delete mode 100644 hooks/test/_util/optionsSpies.js create mode 100644 test/_util/optionSpies.js diff --git a/hooks/test/_util/optionsSpies.js b/hooks/test/_util/optionsSpies.js deleted file mode 100644 index d35d9c2cb1a..00000000000 --- a/hooks/test/_util/optionsSpies.js +++ /dev/null @@ -1,18 +0,0 @@ -import { options } from "preact"; - -let oldBeforeRender = options._render; -let oldAfterDiff = options.diffed; -let oldUnmount = options.unmount; - -/** @type {import('sinon').SinonSpy} */ -export const beforeRenderSpy = sinon.spy(oldBeforeRender); - -/** @type {import('sinon').SinonSpy} */ -export const afterDiffSpy = sinon.spy(oldAfterDiff); - -/** @type {import('sinon').SinonSpy} */ -export const unmountSpy = sinon.spy(oldUnmount); - -options._render = beforeRenderSpy; -options.diffed = afterDiffSpy -options.unmount = unmountSpy; diff --git a/hooks/test/browser/options-hooks.test.js b/hooks/test/browser/options-hooks.test.js index a5ddb9a356c..6398c167665 100644 --- a/hooks/test/browser/options-hooks.test.js +++ b/hooks/test/browser/options-hooks.test.js @@ -2,7 +2,7 @@ import { afterDiffSpy, beforeRenderSpy, unmountSpy -} from "../_util/optionsSpies"; +} from "../../../test/_util/optionSpies"; import { setupRerender } from "preact/test-utils"; import { createElement, render } from "preact"; @@ -32,7 +32,7 @@ describe("hook options", () => { }); function App() { - const [count, setCount] = useState(0); + const [count] = useState(0); return
{count}
; } diff --git a/test/_util/optionSpies.js b/test/_util/optionSpies.js new file mode 100644 index 00000000000..ed1320fd041 --- /dev/null +++ b/test/_util/optionSpies.js @@ -0,0 +1,39 @@ +import { options as rawOptions } from "preact"; + +/** @type {import('../../src/internal').Options} */ +let options = rawOptions; + +let oldVNode = options.vnode; +let oldEvent = options.event; +let oldAfterDiff = options.diffed; +let oldUnmount = options.unmount; + +let oldRoot = options._root; +let oldBeforeDiff = options._diff; +let oldBeforeRender = options._render; +let oldBeforeCommit = options._commit; +let oldHook = options._hook; +let oldCatchError = options._catchError; + +export const vnodeSpy = sinon.spy(oldVNode); +export const eventSpy = sinon.spy(oldEvent); +export const afterDiffSpy = sinon.spy(oldAfterDiff); +export const unmountSpy = sinon.spy(oldUnmount); + +export const rootSpy = sinon.spy(oldRoot); +export const beforeDiffSpy = sinon.spy(oldBeforeDiff); +export const beforeRenderSpy = sinon.spy(oldBeforeRender); +export const beforeCommitSpy = sinon.spy(oldBeforeCommit); +export const hookSpy = sinon.spy(oldHook); +export const catchErrorSpy = sinon.spy(oldCatchError); + +options.vnode = vnodeSpy; +options.event = eventSpy; +options.diffed = afterDiffSpy; +options.unmount = unmountSpy; +options._root = rootSpy; +options._diff = beforeDiffSpy; +options._render = beforeRenderSpy; +options._commit = beforeCommitSpy; +options._hook = hookSpy; +options._catchError = catchErrorSpy; From c7d0bff6ad2b476c3ed0bfac4a49d4c758b87b8b Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 25 Oct 2019 15:01:04 -0700 Subject: [PATCH 3/9] Add tests verifying debug & devtools call old options --- debug/src/debug.js | 6 + debug/test/browser/options-debug.test.js | 130 ++++++++++++++++++++ debug/test/browser/options-devtools.test.js | 100 +++++++++++++++ 3 files changed, 236 insertions(+) create mode 100644 debug/test/browser/options-debug.test.js create mode 100644 debug/test/browser/options-devtools.test.js diff --git a/debug/src/debug.js b/debug/src/debug.js index 5d209e17a12..231f14c0d71 100644 --- a/debug/src/debug.js +++ b/debug/src/debug.js @@ -17,6 +17,8 @@ export function initDebug() { let oldDiffed = options.diffed; let oldVnode = options.vnode; let oldCatchError = options._catchError; + let oldRoot = options._root; + let oldHook = options._hook; const warnedComponents = { useEffect: {}, useLayoutEffect: {}, lazyPropTypes: {} }; options._catchError = (error, vnode, oldVNode) => { @@ -58,6 +60,8 @@ export function initDebug() { Expected a valid HTML node as a second argument to render. Received ${parentNode} instead: render(<${vnode.type.name || vnode.type} />, ${parentNode}); `); + + if (oldRoot) oldRoot(vnode, parentNode); }; options._diff = vnode => { @@ -164,6 +168,8 @@ export function initDebug() { if (!comp) { throw new Error('Hook can only be invoked from render methods.'); } + + if (oldHook) oldHook(comp); }; const warn = (property, err) => ({ diff --git a/debug/test/browser/options-debug.test.js b/debug/test/browser/options-debug.test.js new file mode 100644 index 00000000000..8e40d5b26bc --- /dev/null +++ b/debug/test/browser/options-debug.test.js @@ -0,0 +1,130 @@ +import { + vnodeSpy, + rootSpy, + beforeDiffSpy, + hookSpy, + afterDiffSpy, + catchErrorSpy +} from '../../../test/_util/optionSpies'; + +import { createElement, render, Component } from 'preact'; +import { useState } from 'preact/hooks'; +import { setupRerender } from 'preact/test-utils'; +import { initDebug } from '../../src/debug'; +import { setupScratch, teardown } from '../../../test/_util/helpers'; + +/** @jsx createElement */ + +describe('debug options', () => { + /** @type {HTMLDivElement} */ + let scratch; + + /** @type {() => void} */ + let rerender; + + /** @type {(count: number) => void} */ + let setCount; + + before(() => { + initDebug(); + }); + + beforeEach(() => { + scratch = setupScratch(); + rerender = setupRerender(); + + vnodeSpy.resetHistory(); + rootSpy.resetHistory(); + beforeDiffSpy.resetHistory(); + hookSpy.resetHistory(); + afterDiffSpy.resetHistory(); + catchErrorSpy.resetHistory(); + }); + + afterEach(() => { + teardown(scratch); + }); + + class ClassApp extends Component { + constructor() { + super(); + this.state = { count: 0 }; + setCount = count => this.setState({ count }); + } + + render() { + return
{this.state.count}
; + } + } + + it('should call old options on mount', () => { + render(, scratch); + + expect(vnodeSpy).to.have.been.called; + expect(rootSpy).to.have.been.called; + expect(beforeDiffSpy).to.have.been.called; + expect(afterDiffSpy).to.have.been.called; + }); + + it('should call old options on update', () => { + render(, scratch); + + setCount(1); + rerender(); + + expect(vnodeSpy).to.have.been.called; + expect(rootSpy).to.have.been.called; + expect(beforeDiffSpy).to.have.been.called; + expect(afterDiffSpy).to.have.been.called; + }); + + it('should call old options on unmount', () => { + render(, scratch); + render(null, scratch); + + expect(vnodeSpy).to.have.been.called; + expect(rootSpy).to.have.been.called; + expect(beforeDiffSpy).to.have.been.called; + expect(afterDiffSpy).to.have.been.called; + }); + + it('should call old hook options for hook components', () => { + function HookApp() { + const [count, realSetCount] = useState(0); + setCount = realSetCount; + return
{count}
; + } + + render(, scratch); + + expect(hookSpy).to.have.been.called; + }); + + it('should call old options on error', () => { + class ErrorApp extends Component { + constructor() { + super(); + this.state = { error: true }; + } + componentDidCatch() { + this.setState({ error: false }); + } + render() { + return ; + } + } + + function Throw({ error }) { + if (error) { + throw new Error('test'); + } else { + return
no error
; + } + } + + render(, scratch); + rerender(); + + expect(catchErrorSpy).to.have.been.called; + }); +}); diff --git a/debug/test/browser/options-devtools.test.js b/debug/test/browser/options-devtools.test.js new file mode 100644 index 00000000000..9b5c5a80ec2 --- /dev/null +++ b/debug/test/browser/options-devtools.test.js @@ -0,0 +1,100 @@ +import { + vnodeSpy, + beforeDiffSpy, + afterDiffSpy, + beforeCommitSpy, + unmountSpy +} from '../../../test/_util/optionSpies'; + +import { createElement, render, Component } from 'preact'; +import { useState } from 'preact/hooks'; +import { setupRerender } from 'preact/test-utils'; +import { initDebug } from '../../src/debug'; +import { setupScratch, teardown } from '../../../test/_util/helpers'; + +/** @jsx createElement */ + +describe('debug options', () => { + /** @type {HTMLDivElement} */ + let scratch; + + /** @type {() => void} */ + let rerender; + + /** @type {(count: number) => void} */ + let setCount; + + before(() => { + initDebug(); + }); + + beforeEach(() => { + scratch = setupScratch(); + rerender = setupRerender(); + + vnodeSpy.resetHistory(); + beforeDiffSpy.resetHistory(); + afterDiffSpy.resetHistory(); + beforeCommitSpy.resetHistory(); + unmountSpy.resetHistory(); + }); + + afterEach(() => { + teardown(scratch); + }); + + class ClassApp extends Component { + constructor() { + super(); + this.state = { count: 0 }; + setCount = count => this.setState({ count }); + } + + render() { + return
{this.state.count}
; + } + } + + it('should call old options on mount', () => { + render(, scratch); + + expect(vnodeSpy).to.have.been.called; + expect(beforeDiffSpy).to.have.been.called; + expect(afterDiffSpy).to.have.been.called; + expect(beforeCommitSpy).to.have.been.called; + }); + + it('should call old options on update', () => { + render(, scratch); + + setCount(1); + rerender(); + + expect(vnodeSpy).to.have.been.called; + expect(beforeDiffSpy).to.have.been.called; + expect(afterDiffSpy).to.have.been.called; + expect(beforeCommitSpy).to.have.been.called; + }); + + it('should call old options on unmount', () => { + render(, scratch); + render(null, scratch); + + expect(unmountSpy).to.have.been.called; + }); + + it('should call old hook options for hook components', () => { + function HookApp() { + const [count, realSetCount] = useState(0); + setCount = realSetCount; + return
{count}
; + } + + render(, scratch); + + expect(vnodeSpy).to.have.been.called; + expect(beforeDiffSpy).to.have.been.called; + expect(afterDiffSpy).to.have.been.called; + expect(beforeCommitSpy).to.have.been.called; + }); +}); From 6ca00e599db7541fa42bfce7abcd4e7adcf2bc18 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 25 Oct 2019 16:22:44 -0700 Subject: [PATCH 4/9] Improve compat coverage --- compat/test/browser/compat.test.js | 9 ++++--- compat/test/browser/forwardRef.test.js | 36 ++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/compat/test/browser/compat.test.js b/compat/test/browser/compat.test.js index a658927d200..92760d8cdb2 100644 --- a/compat/test/browser/compat.test.js +++ b/compat/test/browser/compat.test.js @@ -21,9 +21,12 @@ describe('imported compat in preact', () => { scratch.firstChild.click(); expect(spy).to.be.calledOnce; - expect(spy.args[0][0]).to.haveOwnProperty('persist'); - expect(typeof spy.args[0][0].persist).to.equal('function'); - expect(spy.args[0][0]).to.haveOwnProperty('nativeEvent'); + const event = spy.args[0][0]; + expect(event).to.haveOwnProperty('persist'); + expect(event).to.haveOwnProperty('nativeEvent'); + expect(typeof event.persist).to.equal('function'); + + expect(() => event.persist()).to.not.throw(); }); it('should normalize ondoubleclick event', () => { diff --git a/compat/test/browser/forwardRef.test.js b/compat/test/browser/forwardRef.test.js index d872da22e1b..b7a57dc287f 100644 --- a/compat/test/browser/forwardRef.test.js +++ b/compat/test/browser/forwardRef.test.js @@ -239,6 +239,42 @@ describe('forwardRef', () => { expect(renderCount).to.equal(3); }); + it('should bailout if forwardRef is wrapped in memo using function refs', () => { + const Component = props =>
; + + let renderCount = 0; + + const App = memo( + forwardRef((props, ref) => { + renderCount++; + return ; + }), + ); + + const ref = sinon.spy(); + + render(, scratch); + expect(renderCount).to.equal(1); + + expect(ref).to.have.been.called; + + ref.resetHistory(); + render(, scratch); + expect(renderCount).to.equal(1); + + const differentRef = sinon.spy(); + + render(, scratch); + expect(renderCount).to.equal(2); + + expect(ref).to.have.been.calledWith(null); + expect(differentRef).to.have.been.called; + + differentRef.resetHistory(); + render(, scratch); + expect(renderCount).to.equal(3); + }); + it('should pass ref through memo() with custom comparer function', () => { const Foo = props =>
; From da2259af3096dae193482749d25ea46a242bdae2 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 25 Oct 2019 16:52:41 -0700 Subject: [PATCH 5/9] Skip an untested line --- compat/src/index.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compat/src/index.js b/compat/src/index.js index 1501a9a03c8..bc313ffc01f 100644 --- a/compat/src/index.js +++ b/compat/src/index.js @@ -389,6 +389,10 @@ function setSafeDescriptor(proto, key) { Object.defineProperty(proto, key, { configurable: false, get() { return this['UNSAFE_' + key]; }, + // This set is only used if a user sets an a lifecycle like cWU + // after setting a lifecycle like UNSAFE_cWU. I double anyone + // actually does this in practice so not testing it + /* istanbul ignore next */ set(v) { this['UNSAFE_' + key] = v; } }); } From 7a4fbbcf561ef983a843d106f79e2d05e58b29e7 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 25 Oct 2019 16:53:03 -0700 Subject: [PATCH 6/9] Improve coverage of debug --- debug/src/debug.js | 20 +++++++++++--------- debug/test/browser/debug.test.js | 4 ++++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/debug/src/debug.js b/debug/src/debug.js index 231f14c0d71..6b1a8f324d0 100644 --- a/debug/src/debug.js +++ b/debug/src/debug.js @@ -227,15 +227,17 @@ export function initDebug() { if (vnode._component && vnode._component.__hooks) { let hooks = vnode._component.__hooks; - (hooks._list || []).forEach(hook => { - if (hook._callback && (!hook._args || !Array.isArray(hook._args))) { - /* istanbul ignore next */ - console.warn( - `In ${vnode.type.name || vnode.type} you are calling useMemo/useCallback without passing arguments.\n` + - `This is a noop since it will not be able to memoize, it will execute it every render.` - ); - } - }); + if (hooks._list && Array.isArray(hooks._list)) { + hooks._list.forEach(hook => { + if (hook._callback && (!hook._args || !Array.isArray(hook._args))) { + /* istanbul ignore next */ + console.warn( + `In ${vnode.type.name || vnode.type} you are calling useMemo/useCallback without passing arguments.\n` + + `This is a noop since it will not be able to memoize, it will execute it every render.` + ); + } + }); + } if (hooks._pendingEffects && Array.isArray(hooks._pendingEffects)) { hooks._pendingEffects.forEach((effect) => { if ((!effect._args || !Array.isArray(effect._args)) && !warnedComponents.useEffect[vnode.type]) { diff --git a/debug/test/browser/debug.test.js b/debug/test/browser/debug.test.js index 4a801e18365..b635a054482 100644 --- a/debug/test/browser/debug.test.js +++ b/debug/test/browser/debug.test.js @@ -274,6 +274,7 @@ describe('debug', () => { } render(, scratch); + forceUpdate(); expect(console.warn).to.not.be.called; render(null, scratch); @@ -477,6 +478,9 @@ describe('debug', () => { expect(serializeVNode()) .to.equal(''); + + expect(serializeVNode(
)) + .to.equal('
'); }); }); From 703377aee842d1d3c19a2b7121000106c90929ea Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 25 Oct 2019 16:56:26 -0700 Subject: [PATCH 7/9] Fix linting rules --- compat/src/index.js | 2 +- compat/test/browser/memo.test.js | 1 + debug/src/devtools/index.js | 1 + debug/test/browser/options-debug.test.js | 4 ++- debug/test/browser/options-devtools.test.js | 3 ++- hooks/test/browser/options-hooks.test.js | 30 ++++++++++++++------- test/_util/optionSpies.js | 2 +- 7 files changed, 29 insertions(+), 14 deletions(-) diff --git a/compat/src/index.js b/compat/src/index.js index bc313ffc01f..2b692962146 100644 --- a/compat/src/index.js +++ b/compat/src/index.js @@ -390,7 +390,7 @@ function setSafeDescriptor(proto, key) { configurable: false, get() { return this['UNSAFE_' + key]; }, // This set is only used if a user sets an a lifecycle like cWU - // after setting a lifecycle like UNSAFE_cWU. I double anyone + // after setting a lifecycle like UNSAFE_cWU. I doubt anyone // actually does this in practice so not testing it /* istanbul ignore next */ set(v) { this['UNSAFE_' + key] = v; } diff --git a/compat/test/browser/memo.test.js b/compat/test/browser/memo.test.js index bde6288c302..92d9d076f65 100644 --- a/compat/test/browser/memo.test.js +++ b/compat/test/browser/memo.test.js @@ -17,6 +17,7 @@ describe('memo()', () => { }); it('should have isReactComponent flag', () => { + // eslint-disable-next-line react/display-name let App = memo(() =>
foo
); expect(App.prototype.isReactComponent).to.equal(true); }); diff --git a/debug/src/devtools/index.js b/debug/src/devtools/index.js index cb8d6bc3136..a33af9412ac 100644 --- a/debug/src/devtools/index.js +++ b/debug/src/devtools/index.js @@ -73,6 +73,7 @@ export function initDevTools() { }; if (!hook._renderers) { + // eslint-disable-next-line no-console console.info( 'Preact is not compatible with your version of react-devtools. We ' + 'will address this in future releases.' diff --git a/debug/test/browser/options-debug.test.js b/debug/test/browser/options-debug.test.js index 8e40d5b26bc..545d4771708 100644 --- a/debug/test/browser/options-debug.test.js +++ b/debug/test/browser/options-debug.test.js @@ -16,6 +16,7 @@ import { setupScratch, teardown } from '../../../test/_util/helpers'; /** @jsx createElement */ describe('debug options', () => { + /** @type {HTMLDivElement} */ let scratch; @@ -117,7 +118,8 @@ describe('debug options', () => { function Throw({ error }) { if (error) { throw new Error('test'); - } else { + } + else { return
no error
; } } diff --git a/debug/test/browser/options-devtools.test.js b/debug/test/browser/options-devtools.test.js index 9b5c5a80ec2..14fc5b12aab 100644 --- a/debug/test/browser/options-devtools.test.js +++ b/debug/test/browser/options-devtools.test.js @@ -14,7 +14,8 @@ import { setupScratch, teardown } from '../../../test/_util/helpers'; /** @jsx createElement */ -describe('debug options', () => { +describe('devtools options', () => { + /** @type {HTMLDivElement} */ let scratch; diff --git a/hooks/test/browser/options-hooks.test.js b/hooks/test/browser/options-hooks.test.js index 6398c167665..8644edf4f33 100644 --- a/hooks/test/browser/options-hooks.test.js +++ b/hooks/test/browser/options-hooks.test.js @@ -2,22 +2,26 @@ import { afterDiffSpy, beforeRenderSpy, unmountSpy -} from "../../../test/_util/optionSpies"; +} from '../../../test/_util/optionSpies'; -import { setupRerender } from "preact/test-utils"; -import { createElement, render } from "preact"; -import { setupScratch, teardown } from "../../../test/_util/helpers"; -import { useState } from "../../src"; +import { setupRerender } from 'preact/test-utils'; +import { createElement, render } from 'preact'; +import { setupScratch, teardown } from '../../../test/_util/helpers'; +import { useState } from '../../src'; /** @jsx createElement */ -describe("hook options", () => { +describe('hook options', () => { + /** @type {HTMLDivElement} */ let scratch; /** @type {() => void} */ let rerender; + /** @type {() => void} */ + let increment; + beforeEach(() => { scratch = setupScratch(); rerender = setupRerender(); @@ -32,23 +36,29 @@ describe("hook options", () => { }); function App() { - const [count] = useState(0); + const [count, setCount] = useState(0); + increment = () => setCount(prevCount => prevCount + 1); return
{count}
; } - it("should call old options._render", () => { + it('should call old options on mount', () => { render(, scratch); expect(beforeRenderSpy).to.have.been.called; + expect(afterDiffSpy).to.have.been.called; }); - it("should call old options.diffed", () => { + it('should call old options.diffed on update', () => { render(, scratch); + increment(); + rerender(); + + expect(beforeRenderSpy).to.have.been.called; expect(afterDiffSpy).to.have.been.called; }); - it("should call old options.unmount", () => { + it('should call old options on unmount', () => { render(, scratch); render(null, scratch); diff --git a/test/_util/optionSpies.js b/test/_util/optionSpies.js index ed1320fd041..80951ac458a 100644 --- a/test/_util/optionSpies.js +++ b/test/_util/optionSpies.js @@ -1,4 +1,4 @@ -import { options as rawOptions } from "preact"; +import { options as rawOptions } from 'preact'; /** @type {import('../../src/internal').Options} */ let options = rawOptions; From da444b7da05d9eafe875e7fdbc1136980be22ce3 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 25 Oct 2019 17:15:57 -0700 Subject: [PATCH 8/9] Add test for compat calling old options --- compat/src/index.js | 2 - compat/test/browser/options-compat.test.js | 80 ++++++++++++++++++++++ test/_util/optionSpies.js | 2 +- 3 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 compat/test/browser/options-compat.test.js diff --git a/compat/src/index.js b/compat/src/index.js index 2b692962146..86e8fe9a397 100644 --- a/compat/src/index.js +++ b/compat/src/index.js @@ -12,7 +12,6 @@ const CAMEL_PROPS = /^(?:accent|alignment|arabic|baseline|cap|clip|color|fill|fl let oldEventHook = options.event; options.event = e => { - /* istanbul ignore next */ if (oldEventHook) e = oldEventHook(e); e.persist = () => {}; return e.nativeEvent = e; @@ -418,7 +417,6 @@ options.vnode = vnode => { setSafeDescriptor(type.prototype, 'componentWillUpdate'); type._patchedLifecycles = true; } - /* istanbul ignore next */ if (oldVNodeHook) oldVNodeHook(vnode); }; diff --git a/compat/test/browser/options-compat.test.js b/compat/test/browser/options-compat.test.js new file mode 100644 index 00000000000..63de6b63693 --- /dev/null +++ b/compat/test/browser/options-compat.test.js @@ -0,0 +1,80 @@ +import { vnodeSpy, eventSpy } from '../../../test/_util/optionSpies'; + +import { render, createElement, Component, createRef } from '../../src'; +import { setupRerender } from 'preact/test-utils'; +import { setupScratch, teardown } from '../../../test/_util/helpers'; + +/** @jsx createElement */ + +describe('compat options', () => { + + /** @type {HTMLDivElement} */ + let scratch; + + /** @type {() => void} */ + let rerender; + + /** @type {() => void} */ + let increment; + + /** @type {import('../../src/index').PropRef} */ + let buttonRef; + + beforeEach(() => { + scratch = setupScratch(); + rerender = setupRerender(); + + vnodeSpy.resetHistory(); + eventSpy.resetHistory(); + + buttonRef = createRef(); + }); + + afterEach(() => { + teardown(scratch); + }); + + class ClassApp extends Component { + constructor() { + super(); + this.state = { count: 0 }; + increment = () => + this.setState(({ count }) => ({ + count: count + 1 + })); + } + + render() { + return ( + + ); + } + } + + it('should call old options on mount', () => { + render(, scratch); + + expect(vnodeSpy).to.have.been.called; + }); + + it('should call old options on event and update', () => { + render(, scratch); + expect(scratch.innerHTML).to.equal(''); + + buttonRef.current.dispatchEvent(new Event('click')); + rerender(); + expect(scratch.innerHTML).to.equal(''); + + expect(vnodeSpy).to.have.been.called; + expect(eventSpy).to.have.been.called; + }); + + it('should call old options on unmount', () => { + render(, scratch); + render(null, scratch); + + expect(vnodeSpy).to.have.been.called; + }); +}); diff --git a/test/_util/optionSpies.js b/test/_util/optionSpies.js index 80951ac458a..c400bf25910 100644 --- a/test/_util/optionSpies.js +++ b/test/_util/optionSpies.js @@ -4,7 +4,7 @@ import { options as rawOptions } from 'preact'; let options = rawOptions; let oldVNode = options.vnode; -let oldEvent = options.event; +let oldEvent = options.event || (e => e); let oldAfterDiff = options.diffed; let oldUnmount = options.unmount; From 26306a1420ffdd0b1c33ba54a770b80da3170ae4 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 25 Oct 2019 17:31:33 -0700 Subject: [PATCH 9/9] Use getDisplayName in hook warnings --- debug/src/debug.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/debug/src/debug.js b/debug/src/debug.js index 6b1a8f324d0..5092afea6d0 100644 --- a/debug/src/debug.js +++ b/debug/src/debug.js @@ -230,9 +230,8 @@ export function initDebug() { if (hooks._list && Array.isArray(hooks._list)) { hooks._list.forEach(hook => { if (hook._callback && (!hook._args || !Array.isArray(hook._args))) { - /* istanbul ignore next */ console.warn( - `In ${vnode.type.name || vnode.type} you are calling useMemo/useCallback without passing arguments.\n` + + `In ${getDisplayName(vnode)} you are calling useMemo/useCallback without passing arguments.\n` + `This is a noop since it will not be able to memoize, it will execute it every render.` ); } @@ -242,10 +241,9 @@ export function initDebug() { hooks._pendingEffects.forEach((effect) => { if ((!effect._args || !Array.isArray(effect._args)) && !warnedComponents.useEffect[vnode.type]) { warnedComponents.useEffect[vnode.type] = true; - /* istanbul ignore next */ console.warn('You should provide an array of arguments as the second argument to the "useEffect" hook.\n\n' + 'Not doing so will invoke this effect on every render.\n\n' + - 'This effect can be found in the render of ' + (vnode.type.name || vnode.type) + '.'); + 'This effect can be found in the render of ' + getDisplayName(vnode) + '.'); } }); } @@ -253,10 +251,9 @@ export function initDebug() { hooks._pendingLayoutEffects.forEach((layoutEffect) => { if ((!layoutEffect._args || !Array.isArray(layoutEffect._args)) && !warnedComponents.useLayoutEffect[vnode.type]) { warnedComponents.useLayoutEffect[vnode.type] = true; - /* istanbul ignore next */ console.warn('You should provide an array of arguments as the second argument to the "useLayoutEffect" hook.\n\n' + 'Not doing so will invoke this effect on every render.\n\n' + - 'This effect can be found in the render of ' + (vnode.type.name || vnode.type) + '.'); + 'This effect can be found in the render of ' + getDisplayName(vnode) + '.'); } }); }