Merge pull request #2037 from preactjs/feat/improve-set-state-warnings

Add debug warnings when calling `setState` or `forceUpdate` on an unmounted component

debug/src/debug.js

@@ -288,9 +288,35 @@ 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);
 };
 
+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.`
+		);
+	}
+	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);
+};
+
 /**
  * Serialize a vnode tree to a string
  * @param {import('./internal').VNode} vnode

debug/test/browser/debug.test.js

@@ -221,6 +221,68 @@ 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 <div>foo</div>;
+			}
+		}
+
+		render(<Foo />, 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) {
+				super(props);
+				this.forceUpdate();
+			}
+			render() {
+				return <div>foo</div>;
+			}
+		}
+
+		render(<Foo />, scratch);
+		expect(console.warn).to.be.calledOnce;
+		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 <div>foo</div>;
+			}
+		}
+
+		render(<Foo />, 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(<div>{{}}</div>, scratch);
 		expect(fn).to.throw(/not valid/);

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;
 
 /*

test/browser/components.test.js

@@ -129,6 +129,21 @@ describe('Components', () => {
 			}
 
 			expect(() => render(<Foo foo="bar" />, 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(<Foo foo="bar" />, scratch)).not.to.throw();
+			rerender();
 			expect(spy).to.not.be.called;
 		});
 
@@ -624,7 +639,6 @@ describe('Components', () => {
 			'</div><button>First Button</button><button>Second Button</button><button>Third Button</button>');
 	});
 
-
 	// Test for Issue preactjs/preact#254
 	it('should not recycle common class children with different keys', () => {
 		let idx = 0;
@@ -2121,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 <div>{ state.count }</div>;
+				}
+			}
+
+			render(<Foo />, scratch);
+			expect(scratch.innerHTML).to.equal('<div>0</div>');
+
+			increment();
+			rerender();
+			expect(scratch.innerHTML).to.equal('<div>1</div>');
+
+			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 <div>Hello</div>;
+				}
+			}
+
+			render(<Foo />, scratch);
+			expect(scratch.innerHTML).to.equal('<div>Hello</div>');
+
+			render(null, scratch);
+			expect(scratch.innerHTML).to.equal('');
+
+			expect(() => forceUpdate()).to.not.throw();
+			expect(() => rerender()).to.not.throw();
+			expect(scratch.innerHTML).to.equal('');
+		});
+	});
 });