-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Changes from all commits
04076f6
ba82e1d
30b4556
50d7720
d8fa20c
751b1f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,4 +49,4 @@ | |
"eslint-plugin-jsx-a11y": "^6.0.3", | ||
"eslint-plugin-react": "^7.5.1" | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, weird. maybe npm itself. not a big deal. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 But all tests are passing in my environment. The reason is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I'll dig into it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-helper/package.json#L50 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
||
|
@@ -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] : []); | ||
}); | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a test case for this? or does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, |
||
instance.setState(state); | ||
if ( | ||
shouldRender && | ||
!this[OPTIONS].disableLifecycleMethods && | ||
|
@@ -400,6 +402,10 @@ class ShallowWrapper { | |
} | ||
} | ||
this.update(); | ||
// call the setState callback | ||
if (callback) { | ||
adapter.invokeSetStateCallback(instance, callback); | ||
} | ||
}); | ||
}); | ||
return this; | ||
|
There was a problem hiding this comment.
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 fromifReact
.https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-helper/src/ifReact.js#L7
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.