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

Bug fix - SetState callback called before component state is updated in ReactShallowRenderer #11507

Merged
merged 17 commits into from Nov 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
035eb3d
Create test to verify ReactShallowRenderer bug (#11496)
accordeiro Nov 9, 2017
856cb2b
Fix ReactShallowRenderer callback bug on componentWillMount (#11496)
accordeiro Nov 9, 2017
a269747
Improve fnction naming and clean up queued callback before call
accordeiro Nov 9, 2017
a13a3d6
Merge branch 'master' into shallow-renderer-bug
accordeiro Nov 9, 2017
11b931d
Run prettier on ReactShallowRenderer.js
accordeiro Nov 10, 2017
ae88abd
Consolidate callback call on ReactShallowRenderer.js
accordeiro Nov 10, 2017
87bf19f
Ensure callback behavior is similar between ReactDOM and ReactShallow…
accordeiro Nov 13, 2017
120b399
Fix Code Review requests (#11507)
accordeiro Nov 13, 2017
cfa6552
Move test to ReactCompositeComponent
gaearon Nov 13, 2017
ffa5565
Verify the callback gets called
gaearon Nov 13, 2017
f242683
Ensure multiple callbacks are correctly handled on ReactShallowRenderer
accordeiro Nov 19, 2017
de20f22
Merge branch 'shallow-renderer-bug' of github.com:accordeiro/react in…
accordeiro Nov 19, 2017
194ff74
Ensure the setState callback is called inside componentWillMount (Rea…
accordeiro Nov 19, 2017
2141b8c
Clear ReactShallowRenderer callback queue before actually calling the…
accordeiro Nov 22, 2017
b3b0788
Add test for multiple callbacks on ReactShallowRenderer
accordeiro Nov 22, 2017
0a58682
Ensure the ReactShallowRenderer callback queue is cleared after invok…
accordeiro Nov 22, 2017
ba0e995
Remove references to internal fields on ReactShallowRenderer test
accordeiro Nov 22, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 67 additions & 0 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Expand Up @@ -1591,6 +1591,73 @@ describe('ReactCompositeComponent', () => {
expect(mockArgs.length).toEqual(0);
});

it('this.state should be updated on setState callback inside componentWillMount', () => {
const div = document.createElement('div');
let stateSuccessfullyUpdated = false;

class Component extends React.Component {
constructor(props, context) {
super(props, context);
this.state = {
hasUpdatedState: false,
};
}

componentWillMount() {
this.setState(
{hasUpdatedState: true},
() => (stateSuccessfullyUpdated = this.state.hasUpdatedState),
);
}

render() {
return <div>{this.props.children}</div>;
}
}

ReactDOM.render(<Component />, div);
expect(stateSuccessfullyUpdated).toBe(true);
});

it('should call the setState callback even if shouldComponentUpdate = false', done => {
const mockFn = jest.fn().mockReturnValue(false);
const div = document.createElement('div');

let instance;

class Component extends React.Component {
constructor(props, context) {
super(props, context);
this.state = {
hasUpdatedState: false,
};
}

componentWillMount() {
instance = this;
}

shouldComponentUpdate() {
return mockFn();
}

render() {
return <div>{this.state.hasUpdatedState}</div>;
}
}

ReactDOM.render(<Component />, div);

expect(instance).toBeDefined();
expect(mockFn).not.toBeCalled();

instance.setState({hasUpdatedState: true}, () => {
expect(mockFn).toBeCalled();
expect(instance.state.hasUpdatedState).toBe(true);
done();
});
});

it('should return a meaningful warning when constructor is returned', () => {
spyOn(console, 'error');
class RenderTextInvalidConstructor extends React.Component {
Expand Down
35 changes: 23 additions & 12 deletions packages/react-test-renderer/src/ReactShallowRenderer.js
Expand Up @@ -103,6 +103,7 @@ class ReactShallowRenderer {
}

this._rendering = false;
this._updater._invokeCallbacks();

return this.getRenderOutput();
}
Expand Down Expand Up @@ -192,31 +193,45 @@ class ReactShallowRenderer {
class Updater {
constructor(renderer) {
this._renderer = renderer;
this._callbacks = [];
}

_enqueueCallback(callback, publicInstance) {
if (typeof callback === 'function' && publicInstance) {
this._callbacks.push({
callback,
publicInstance,
});
}
}

_invokeCallbacks() {
const callbacks = this._callbacks;
this._callbacks = [];

callbacks.forEach(({callback, publicInstance}) => {
callback.call(publicInstance);
});
}

isMounted(publicInstance) {
return !!this._renderer._element;
}

enqueueForceUpdate(publicInstance, callback, callerName) {
this._enqueueCallback(callback, publicInstance);
this._renderer._forcedUpdate = true;
this._renderer.render(this._renderer._element, this._renderer._context);

if (typeof callback === 'function') {
callback.call(publicInstance);
}
}

enqueueReplaceState(publicInstance, completeState, callback, callerName) {
this._enqueueCallback(callback, publicInstance);
this._renderer._newState = completeState;
this._renderer.render(this._renderer._element, this._renderer._context);

if (typeof callback === 'function') {
callback.call(publicInstance);
}
}

enqueueSetState(publicInstance, partialState, callback, callerName) {
this._enqueueCallback(callback, publicInstance);
const currentState = this._renderer._newState || publicInstance.state;

if (typeof partialState === 'function') {
Expand All @@ -229,10 +244,6 @@ class Updater {
};

this._renderer.render(this._renderer._element, this._renderer._context);

if (typeof callback === 'function') {
callback.call(publicInstance);
}
}
}

Expand Down
Expand Up @@ -845,6 +845,97 @@ describe('ReactShallowRenderer', () => {
expect(result).toEqual(<div>baz:bar</div>);
});

it('this.state should be updated on setState callback inside componentWillMount', () => {
let stateSuccessfullyUpdated = false;

class Component extends React.Component {
constructor(props, context) {
super(props, context);
this.state = {
hasUpdatedState: false,
};
}

componentWillMount() {
this.setState(
{hasUpdatedState: true},
() => (stateSuccessfullyUpdated = this.state.hasUpdatedState),
);
}

render() {
return <div>{this.props.children}</div>;
}
}

const shallowRenderer = createRenderer();
shallowRenderer.render(<Component />);
expect(stateSuccessfullyUpdated).toBe(true);
});

it('should handle multiple callbacks', () => {
const mockFn = jest.fn();
const shallowRenderer = createRenderer();

class Component extends React.Component {
constructor(props, context) {
super(props, context);
this.state = {
foo: 'foo',
};
}

componentWillMount() {
this.setState({foo: 'bar'}, () => mockFn());
this.setState({foo: 'foobar'}, () => mockFn());
}

render() {
return <div>{this.state.foo}</div>;
}
}

shallowRenderer.render(<Component />);

expect(mockFn.mock.calls.length).toBe(2);

// Ensure the callback queue is cleared after the callbacks are invoked
const mountedInstance = shallowRenderer.getMountedInstance();
mountedInstance.setState({foo: 'bar'}, () => mockFn());
expect(mockFn.mock.calls.length).toBe(3);
});

it('should call the setState callback even if shouldComponentUpdate = false', done => {
const mockFn = jest.fn().mockReturnValue(false);

class Component extends React.Component {
constructor(props, context) {
super(props, context);
this.state = {
hasUpdatedState: false,
};
}

shouldComponentUpdate() {
return mockFn();
}

render() {
return <div>{this.state.hasUpdatedState}</div>;
}
}

const shallowRenderer = createRenderer();
shallowRenderer.render(<Component />);

const mountedInstance = shallowRenderer.getMountedInstance();
mountedInstance.setState({hasUpdatedState: true}, () => {
expect(mockFn).toBeCalled();
expect(mountedInstance.state.hasUpdatedState).toBe(true);
done();
});
});

it('throws usefully when rendering badly-typed elements', () => {
spyOn(console, 'error');
const shallowRenderer = createRenderer();
Expand Down