From 8218d68459b511d884068e3b4bd9783b74268378 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 24 Oct 2019 00:20:49 -0700 Subject: [PATCH 1/4] Ignore defer definition in coverage --- src/component.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/component.js b/src/component.js index db2c0d304b..513a2c4e8a 100644 --- a/src/component.js +++ b/src/component.js @@ -150,6 +150,7 @@ let q = []; * Asynchronously schedule a callback * @type {(cb) => void} */ +/* istanbul ignore next */ const defer = typeof Promise=='function' ? Promise.prototype.then.bind(Promise.resolve()) : setTimeout; /* From 71f8e8df6ed8b5ade8ffb562dbc41f347cbc8117 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 24 Oct 2019 00:21:03 -0700 Subject: [PATCH 2/4] Warn if forceUpdate is called in constructor --- debug/src/debug.js | 11 +++++++++++ debug/test/browser/debug.test.js | 16 ++++++++++++++++ test/browser/components.test.js | 15 +++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/debug/src/debug.js b/debug/src/debug.js index 6e259b9e83..45decaf181 100644 --- a/debug/src/debug.js +++ b/debug/src/debug.js @@ -291,6 +291,17 @@ Component.prototype.setState = function(update, callback) { return setState.call(this, update, callback); }; +const forceUpdate = Component.prototype.forceUpdate; +Component.prototype.forceUpdate = function(callback) { + if (this._vnode==null) { + console.warn( + `Calling "this.forceUpdate" inside the constructor of a component is a ` + + `no-op and might be a bug in your application.` + ); + } + return forceUpdate.call(this, callback); +}; + /** * Serialize a vnode tree to a string * @param {import('./internal').VNode} vnode diff --git a/debug/test/browser/debug.test.js b/debug/test/browser/debug.test.js index 1a7a35384e..57cf39b21d 100644 --- a/debug/test/browser/debug.test.js +++ b/debug/test/browser/debug.test.js @@ -221,6 +221,22 @@ describe('debug', () => { expect(console.warn.args[0]).to.match(/no-op/); }); + it('should warn when calling forceUpdate inside the constructor', () => { + class Foo extends Component { + constructor(props) { + super(props); + this.forceUpdate(); + } + render() { + return
foo
; + } + } + + render(, scratch); + expect(console.warn).to.be.calledOnce; + expect(console.warn.args[0]).to.match(/no-op/); + }); + it('should print an error when child is a plain object', () => { let fn = () => render(
{{}}
, scratch); expect(fn).to.throw(/not valid/); diff --git a/test/browser/components.test.js b/test/browser/components.test.js index 2ffc032b6a..83b099773b 100644 --- a/test/browser/components.test.js +++ b/test/browser/components.test.js @@ -129,6 +129,21 @@ describe('Components', () => { } expect(() => render(, scratch)).not.to.throw(); + rerender(); + expect(spy).to.not.be.called; + }); + + it('should not crash when calling forceUpdate with cb in constructor', () => { + let spy = sinon.spy(); + class Foo extends Component { + constructor(props) { + super(props); + this.forceUpdate(spy); + } + } + + expect(() => render(, scratch)).not.to.throw(); + rerender(); expect(spy).to.not.be.called; }); From f97a394e81947eb7f8ab312eaecf1d739684cf4f Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 24 Oct 2019 00:51:32 -0700 Subject: [PATCH 3/4] Add tests verifying setState and forceUpdate don't error when called on an unmounted component --- test/browser/components.test.js | 62 ++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/test/browser/components.test.js b/test/browser/components.test.js index 83b099773b..5fbf4f180a 100644 --- a/test/browser/components.test.js +++ b/test/browser/components.test.js @@ -639,7 +639,6 @@ describe('Components', () => { ''); }); - // Test for Issue preactjs/preact#254 it('should not recycle common class children with different keys', () => { let idx = 0; @@ -2136,4 +2135,65 @@ describe('Components', () => { expect(child.base).to.equalNode(scratch.firstChild.firstChild, 'swapChildTag - child.base'); }); }); + + describe('setState', () => { + it('should not error if called on an unmounted component', () => { + + /** @type {() => void} */ + let increment; + + class Foo extends Component { + constructor(props) { + super(props); + this.state = { count: 0 }; + increment = () => this.setState({ count: this.state.count + 1 }); + } + render(props, state) { + return
{ state.count }
; + } + } + + render(, scratch); + expect(scratch.innerHTML).to.equal('
0
'); + + increment(); + rerender(); + expect(scratch.innerHTML).to.equal('
1
'); + + render(null, scratch); + expect(scratch.innerHTML).to.equal(''); + + expect(() => increment()).to.not.throw(); + expect(() => rerender()).to.not.throw(); + expect(scratch.innerHTML).to.equal(''); + }); + }); + + describe('forceUpdate', () => { + it('should not error if called on an unmounted component', () => { + + /** @type {() => void} */ + let forceUpdate; + + class Foo extends Component { + constructor(props) { + super(props); + forceUpdate = () => this.forceUpdate(); + } + render(props, state) { + return
Hello
; + } + } + + render(, scratch); + expect(scratch.innerHTML).to.equal('
Hello
'); + + render(null, scratch); + expect(scratch.innerHTML).to.equal(''); + + expect(() => forceUpdate()).to.not.throw(); + expect(() => rerender()).to.not.throw(); + expect(scratch.innerHTML).to.equal(''); + }); + }); }); From 1d799c56e9bb9f311b95c2194d40a75f54cf14f6 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 24 Oct 2019 01:00:56 -0700 Subject: [PATCH 4/4] Warn if setState or forceUpdate are called on an unmounted component --- debug/src/debug.js | 15 +++++++++++ debug/test/browser/debug.test.js | 46 ++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/debug/src/debug.js b/debug/src/debug.js index 45decaf181..5d209e17a1 100644 --- a/debug/src/debug.js +++ b/debug/src/debug.js @@ -288,6 +288,14 @@ Component.prototype.setState = function(update, callback) { `"this.state = {}" directly.` ); } + else if (this._parentDom==null) { + console.warn( + `Can't call "this.setState" on an unmounted component. This is a no-op, ` + + `but it indicates a memory leak in your application. To fix, cancel all ` + + `subscriptions and asynchronous tasks in the componentWillUnmount method.` + ); + } + return setState.call(this, update, callback); }; @@ -299,6 +307,13 @@ Component.prototype.forceUpdate = function(callback) { `no-op and might be a bug in your application.` ); } + else if (this._parentDom==null) { + console.warn( + `Can't call "this.setState" on an unmounted component. This is a no-op, ` + + `but it indicates a memory leak in your application. To fix, cancel all ` + + `subscriptions and asynchronous tasks in the componentWillUnmount method.` + ); + } return forceUpdate.call(this, callback); }; diff --git a/debug/test/browser/debug.test.js b/debug/test/browser/debug.test.js index 57cf39b21d..4a801e1836 100644 --- a/debug/test/browser/debug.test.js +++ b/debug/test/browser/debug.test.js @@ -221,6 +221,29 @@ describe('debug', () => { expect(console.warn.args[0]).to.match(/no-op/); }); + it('should warn when calling setState on an unmounted Component', () => { + let setState; + + class Foo extends Component { + constructor(props) { + super(props); + setState = () => this.setState({ foo: true }); + } + render() { + return
foo
; + } + } + + render(, scratch); + expect(console.warn).to.not.be.called; + + render(null, scratch); + + setState(); + expect(console.warn).to.be.calledOnce; + expect(console.warn.args[0]).to.match(/no-op/); + }); + it('should warn when calling forceUpdate inside the constructor', () => { class Foo extends Component { constructor(props) { @@ -237,6 +260,29 @@ describe('debug', () => { expect(console.warn.args[0]).to.match(/no-op/); }); + it('should warn when calling forceUpdate on an unmounted Component', () => { + let forceUpdate; + + class Foo extends Component { + constructor(props) { + super(props); + forceUpdate = () => this.forceUpdate(); + } + render() { + return
foo
; + } + } + + render(, scratch); + expect(console.warn).to.not.be.called; + + render(null, scratch); + + forceUpdate(); + expect(console.warn).to.be.calledOnce; + expect(console.warn.args[0]).to.match(/no-op/); + }); + it('should print an error when child is a plain object', () => { let fn = () => render(
{{}}
, scratch); expect(fn).to.throw(/not valid/);