Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Oct 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions debug/src/debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
62 changes: 62 additions & 0 deletions debug/test/browser/debug.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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/);
Expand Down
1 change: 1 addition & 0 deletions src/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/*
Expand Down
77 changes: 76 additions & 1 deletion test/browser/components.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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('');
});
});
});