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

Fix to call a setState callback after finishing the the render #1453

Merged
merged 6 commits into from
Jan 6, 2018
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
1 change: 1 addition & 0 deletions packages/enzyme-adapter-react-15.4/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"author": "Leland Richardson <leland.richardson@airbnb.com>",
"license": "MIT",
"dependencies": {
"enzyme-adapter-react-helper": "^1.2.2",
"enzyme-adapter-utils": "^1.3.0",
"lodash": "^4.17.4",
"object.assign": "^4.1.0",
Expand Down
10 changes: 10 additions & 0 deletions packages/enzyme-adapter-react-15.4/src/ReactFifteenFourAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
createMountWrapper,
propsWithKeysAndRef,
} from 'enzyme-adapter-utils';
import ifReact from 'enzyme-adapter-react-helper/build/ifReact';

function compositeTypeToNodeType(type) {
switch (type) {
Expand Down Expand Up @@ -252,6 +253,15 @@ class ReactFifteenFourAdapter extends EnzymeAdapter {
createElement(...args) {
return React.createElement(...args);
}

invokeSetStateCallback(instance, callback) {
// React in >= 15.4, and < 16 pass undefined to a setState callback
ifReact(
'^15.4',
() => { callback.call(instance, undefined); },
() => { super.invokeSetStateCallback(instance, callback); },
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ifReact returns a function, not calling it so you have to call a function which is returned from ifReact.

https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-helper/src/ifReact.js#L7

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, oops. shouldn't tests have failed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix in a followup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so we should add a test to guarantee to call a setState callback.
Currently, we don't have a test for it.

}
}

module.exports = ReactFifteenFourAdapter;
5 changes: 5 additions & 0 deletions packages/enzyme-adapter-react-15/src/ReactFifteenAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,11 @@ class ReactFifteenAdapter extends EnzymeAdapter {
createElement(...args) {
return React.createElement(...args);
}

invokeSetStateCallback(instance, callback) {
// React in >= 15.4, and < 16 pass undefined to a setState callback
callback.call(instance, undefined);
}
}

module.exports = ReactFifteenAdapter;
2 changes: 1 addition & 1 deletion packages/enzyme-test-suite/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,4 @@
"eslint-plugin-jsx-a11y": "^6.0.3",
"eslint-plugin-react": "^7.5.1"
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's revert this file; every file should always have a trailing newline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff is added again😉 I'm not digging into that but I guess it is added by a script to run tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, weird. maybe npm itself. not a big deal.

10 changes: 8 additions & 2 deletions packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@ import { ITERATOR_SYMBOL, withSetStateAllowed, sym } from 'enzyme/build/Utils';
import './_helpers/setupAdapters';
import { createClass } from './_helpers/react-compat';
import { describeIf, itIf, itWithData, generateEmptyRenderData } from './_helpers';
import { REACT013, REACT014, REACT16, is } from './_helpers/version';
import { REACT013, REACT014, REACT15, REACT150_4, REACT16, is } from './_helpers/version';

// The shallow renderer in react 16 does not yet support batched updates. When it does,
// we should be able to go un-skip all of the tests that are skipped with this flag.
const BATCHING = !REACT16;

// some React versions pass undefined as an argument of setState callback.
const CALLING_SETSTATE_CALLBACK_WITH_UNDEFINED = REACT15 && !REACT150_4;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this condition doesn't cover a case that React v15.4 calls setState callback with undefined.

But all tests are passing in my environment. The reason is ifReact is checking with React v16.2 because enzyme-adapter-react-helper has React v16.2 into its node_modules.
I'm not sure why enzyme-adapter-react-helper has React v16.2 into its node_modules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it has a peer dep/dev dep on react >= 0.13, so that'd install the latest, but i'm not sure why that would do it. let me try something tho

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll dig into it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enzyme-adapter-react-helper shouldn't have react as a devDependencies?

https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-helper/package.json#L50

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since it's a peer dep, it also needs to be a dev dep.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other packages don't have react as a devDeps. Why does only this package have it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bug leftover from the v3 migration; they all should - every peer dep in every npm package anywhere must always have the identical peer dep string as a dep or a dev dep.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've almost got a fix PR with a regression test; i'll put it up shortly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


const getElementPropSelector = prop => x => x.props[prop];
const getWrapperPropSelector = prop => x => x.prop(prop);

Expand Down Expand Up @@ -1378,8 +1381,11 @@ describe('shallow', () => {
}
const wrapper = shallow(<Foo />);
expect(wrapper.state()).to.eql({ id: 'foo' });
wrapper.setState({ id: 'bar' }, () => {
wrapper.setState({ id: 'bar' }, function callback(...args) {
expect(wrapper.state()).to.eql({ id: 'bar' });
expect(this.state).to.eql({ id: 'bar' });
expect(wrapper.find('div').prop('className')).to.eql('bar');
expect(args).to.eql(CALLING_SETSTATE_CALLBACK_WITH_UNDEFINED ? [undefined] : []);
});
});

Expand Down
18 changes: 12 additions & 6 deletions packages/enzyme-test-suite/test/_helpers/setupAdapters.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,26 @@
* version of React is loaded, and configures enzyme to use the right
* corresponding adapter.
*/
const Version = require('./version');
const {
REACT013,
REACT014,
REACT15,
REACT155,
REACT16,
} = require('./version');
const Enzyme = require('enzyme');

let Adapter = null;

if (Version.REACT013) {
if (REACT013) {
Adapter = require('enzyme-adapter-react-13');
} else if (Version.REACT014) {
} else if (REACT014) {
Adapter = require('enzyme-adapter-react-14');
} else if (Version.REACT155) {
} else if (REACT155) {
Adapter = require('enzyme-adapter-react-15');
} else if (Version.REACT15) {
} else if (REACT15) {
Adapter = require('enzyme-adapter-react-15.4');
} else if (Version.REACT16) {
} else if (REACT16) {
Adapter = require('enzyme-adapter-react-16');
}

Expand Down
1 change: 1 addition & 0 deletions packages/enzyme-test-suite/test/_helpers/version.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const [major, minor] = VERSION.split('.');
export const REACT013 = VERSION.slice(0, 4) === '0.13';
export const REACT014 = VERSION.slice(0, 4) === '0.14';
export const REACT15 = major === '15';
export const REACT150_4 = REACT15 && minor < 5;
export const REACT155 = REACT15 && minor >= 5;
export const REACT16 = major === '16';

Expand Down
5 changes: 5 additions & 0 deletions packages/enzyme/src/EnzymeAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ class EnzymeAdapter {
createElement(type, props, ...children) {
throw unimplementedError('createElement', 'EnzymeAdapter');
}

// eslint-disable-next-line class-methods-use-this
invokeSetStateCallback(instance, callback) {
callback.call(instance);
}
}

EnzymeAdapter.MODES = {
Expand Down
8 changes: 7 additions & 1 deletion packages/enzyme/src/ShallowWrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,9 @@ class ShallowWrapper {
return shouldRender;
};
}
instance.setState(state, callback);
// We don't pass the setState callback here
// to guarantee to call the callback after finishing the render
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test case for this? or does expect(wrapper.find('div').prop('className')).to.eql('bar'); cover that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, expect(wrapper.find('div').prop('className')).to.eql('bar'); is a test for this.

instance.setState(state);
if (
shouldRender &&
!this[OPTIONS].disableLifecycleMethods &&
Expand All @@ -400,6 +402,10 @@ class ShallowWrapper {
}
}
this.update();
// call the setState callback
if (callback) {
adapter.invokeSetStateCallback(instance, callback);
}
});
});
return this;
Expand Down