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.

Sign up for GitHub

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

Jump to bottom

Fix to call componentDidUpdate on setState of React v16 #1261

Merged
merged 1 commit into from
Nov 3, 2017
Merged

Fix to call componentDidUpdate on setState of React v16 #1261

merged 1 commit into from
Nov 3, 2017

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented Oct 13, 2017 •
edited
Loading

@ljharb @lelandrichardson This is to fix #1247 🙏
Currently, componentDidUpdate is not called when calling setState with React v16.

ljharb
ljharb approved these changes Oct 13, 2017
View reviewed changes
packages/enzyme/src/ShallowWrapper.js
@@ -387,7 +387,7 @@ class ShallowWrapper {
// so we replace shouldComponentUpdate to know the result and restore it later.
let originalShouldComponentUpdate;
if (
this[OPTIONS].lifecycleExperimental &&
Copy link
Member

Choose a reason for hiding this comment

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

hm, i wonder if this is causing a number of other bugs related to lifecycle methods?

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 can't find issues causing by this in enzyme repository, but there might be in outside.

@koba04 koba04 mentioned this pull request Oct 14, 2017
Closed
ljharb
ljharb requested changes Oct 16, 2017
View reviewed changes
packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated
@@ -1026,7 +1026,7 @@ describe('shallow', () => {
expect(wrapper.first('div').text()).to.equal('yolo');
});

it('should call componentWillReceiveProps, shouldComponentUpdate and componentWillUpdate with merged newProps', () => {
it('should call componentWillReceiveProps, shouldComponentUpdate and componentWillUpdate and componentDidUpdate with merged newProps', () => {
Copy link
Member

Choose a reason for hiding this comment

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

You have a duplicate "and" here (before both the ultimate and penultimate items); also please use an Oxford comma :-)

@ljharb ljharb requested a review from a team October 17, 2017 02:59
@koba04 koba04 force-pushed the fix-cDU-on-setState branch 2 times, most recently from 841febc to 40e38aa Compare October 18, 2017 09:37
@koba04 koba04 mentioned this pull request Oct 23, 2017
Merged
@ljharb
Copy link
Member

ljharb commented Nov 3, 2017

@koba04 this needs to be rebased, so as to remove all merge commits; i have some commits i need to add to master first, so i'll rebase this branch for you after that, and then merge it.

@ljharb ljharb self-assigned this Nov 3, 2017
@koba04 koba04 force-pushed the fix-cDU-on-setState branch from ca9b440 to d8b83cd Compare November 3, 2017 14:04
@koba04
Copy link
Contributor Author

koba04 commented Nov 3, 2017

@ljharb Rebased!

But I couldn't run unit tests on my local because lerna bootstap never finish at the following point... Is it just me?

lerna info version 2.5.1
lerna info versioning independent
lerna info Bootstrapping 9 packages
lerna info lifecycle preinstall
lerna info Installing external dependencies
⸨         ░░░░░░░░░⸩ ⠙ install dependencies: verb actions 9 actions, concurrency 4

I'm using macOS 10.12.6 and Node v8.9.0 and npm v5.5.1.

@koba04 koba04 mentioned this pull request Nov 3, 2017
Merged
@ljharb ljharb force-pushed the fix-cDU-on-setState branch from d8b83cd to 4ab4804 Compare November 3, 2017 17:19
@ljharb
Copy link
Member

ljharb commented Nov 3, 2017

@koba04 hm, that should definitely work. i'll give it a shot.

@ljharb
Copy link
Member

ljharb commented Nov 3, 2017

(altho you don't need to run lerna yourself; npm install should do all the necessary things)

@koba04
Copy link
Contributor Author

koba04 commented Nov 3, 2017

Thanks! To be accurate, that is a result through npm install. If anyone doesn't have the trouble, my environment would have a problem.

@ljharb ljharb merged commit 4ab4804 into enzymejs:master Nov 3, 2017
@koba04
Copy link
Contributor Author

koba04 commented Nov 4, 2017

Thanks!

@koba04 koba04 deleted the fix-cDU-on-setState branch November 4, 2017 01:44
@koba04
Copy link
Contributor Author

koba04 commented Nov 4, 2017

#1261 (comment)

(I avoided the trouble by lerna bootstrap --concurrency 1 🤔)

This was referenced Nov 5, 2017
Merged
Closed
@dependencies dependencies bot mentioned this pull request Dec 5, 2017
Merged
@dependencies dependencies bot mentioned this pull request Mar 1, 2018
Open
@luissmg
Copy link

luissmg commented May 24, 2018

Is this fixed and already in the latest version? Because I am using 3.3.0 and this problem persists. The workaround for me is to use mount instead of shallow.

@koba04
Copy link
Contributor Author

koba04 commented May 25, 2018

@luissmg Yes, I think so. If you still have an issue, please create an issue with an example to reproduce it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lelandrichardson lelandrichardson lelandrichardson approved these changes

@ljharb ljharb ljharb approved these changes

@nfcampos nfcampos Awaiting requested review from nfcampos

@blainekasten blainekasten Awaiting requested review from blainekasten

@aweary aweary Awaiting requested review from aweary

Assignees

@ljharb ljharb

Labels
API: shallow package: react adapter: 16 semver: patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lifecycle methods aren't called as expected when state changes
4 participants
@koba04 @ljharb @luissmg @lelandrichardson