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

Jump to bottom

[Fix] shallow: ensure SCU is called after GDSFP returns null #1981

Merged
merged 4 commits into from Mar 12, 2019
Merged

[Fix] shallow: ensure SCU is called after GDSFP returns null #1981

merged 4 commits into from Mar 12, 2019

Conversation

chenesan
Copy link
Contributor

@chenesan chenesan commented Jan 18, 2019 •
edited by ljharb

Prevent forgetting PR for #1970 😓 . I've added test case in this PR(should fail now).
Wait for release of facebook/react#14613 and we can update react-test-renderer in dependency.

Fixes #1970.

@ljharb
Copy link
Member

ljharb commented Jan 19, 2019

Thanks! Can we add the same test case for mount as well?

@chenesan
Copy link
Contributor Author

chenesan commented Feb 7, 2019

@ljharb react 16.8 is out. I have tried manually upgrade react-test-renderer to 16.8 in enzyme-react-adapter-16 and the tests passed :-) but I don't know how to handle the react-test-renderer dependency in enzyme.

@ljharb
Copy link
Member

ljharb commented Feb 7, 2019

@chenesan i'm not sure what you mean; it should be auto installed

@ljharb
Copy link
Member

ljharb commented Feb 7, 2019

The error showing up in the tests is specific to React 16.3. It seems like this might be fixable in the 16.3 adapter?

@chenesan
Copy link
Contributor Author

chenesan commented Feb 7, 2019 •
edited

@ljharb I think it's not fixable in 16.3 because it's fixed in react-test-renderer@16.8. I'll add an additional itIf(is('>= 16.8')) for the test.

I don't know how to handle the react-test-renderer dependency in enzyme.

At that time I noticed the react-test-renderer was still 16.7.0 after I run npm run react 16. After rm -rf node_modules and rerun npm install && npm run react 16 it's auto updated to 16.8.1 so it's fine now. Sorry for confusion.

@chenesan
Copy link
Contributor Author

chenesan commented Feb 8, 2019

@ljharb I saw that the build failed in React=16.0 and node=4 after my push. I looked into the log and it failed in before_install step. I'm certain that there's no dependency change in this PR so I'm wondering why that happened.

@ljharb
Copy link
Member

ljharb commented Feb 8, 2019

@chenesan i'd prefer if we could add whatever changes are necessary to fix it in 16.3, with react-test-renderer 16.3. We've done similar fixes before.

As for the correct version of react-test-renderer, that's something I might have to fix - testing should be done with react-test-renderer at the same minor version as react itself.

@chenesan
Copy link
Contributor Author

chenesan commented Feb 9, 2019

@ljharb Ok, I'll try to fix this in adapter 16.3. However, since the lifecycle logic is running in the react shallow renderer, I think it would be hard or not possible to fix this (maybe I'm wrong).

We've done similar fixes before.

Could you guide me where the related issues / fixes are? Maybe I can get some ideas from this. Thanks :-)

@ljharb
Copy link
Member

ljharb commented Feb 9, 2019

Mainly, we can spy on component methods, so that we should be able to determine when individual lifecycle methods occur.

@chenesan chenesan force-pushed the test-gDSFP-before-sCP branch 2 times, most recently from 552714e to 9e2b05e Compare February 28, 2019 10:06
@chenesan
Copy link
Contributor Author

@ljharb I think this could be reviewed now :). I tried to wrap sCU and set instance state as correct old state so this.state won't be the state updated after gDSFP in shallow renderer (which should be set after sCU).

@ljharb
Copy link
Member

ljharb commented Mar 11, 2019

I've updated/rebased this; but it seems like facebook/react@4f33288 doesn't actually fix the bug in react-test-renderer 16.8+ - once I made it so that the fix only applied to 16.3 - 16.7, the 16.8 tests failed locally :-/ altho everything seems to pass in CI, so I'm a bit confused.

@chenesan
Copy link
Contributor Author

Hmm interesting :( I'd try to take a look in this week.

@ljharb ljharb changed the title [TEST] add test case for state diffing in SCU after GDSFP [Fix] shallow: ensure SCU is called after GDSFP returns null Mar 12, 2019
@ljharb
Copy link
Member

ljharb commented Mar 12, 2019 •
edited

k, figured it out. The bug is dependent on react-test-renderer version; so i had to fix the logic to pivot based on that.

@ljharb ljharb merged commit 98154a9 into enzymejs:master Mar 12, 2019
@ljharb ljharb mentioned this pull request Mar 12, 2019
Merged
ljharb added a commit that referenced this pull request Mar 13, 2019
@ljharb
[enzyme-adapter-react-16.3] v1.6.2
ad1a3db 
 - [fix] add `hasShouldComponentUpdateBug` to gDSFP lifecycles option (#1981))
 - [refactor] use `displayNameOfNode` instead of `function.prototype.name` directly
 - [meta] add "directory" field to package.json
 - [deps] update `react-is`, `enzyme-adapter-utils`, `prop-types`
ljharb added a commit that referenced this pull request Mar 13, 2019
@ljharb
[enzyme-adapter-react-16] v1.11.0
534839f 
 - [new] Wrap renders in `TestUtils.act` (#2034)
 - [fix] `shallow`: Mock sCU if gDSFP defined in shallow renderer rerender (#1981)
 - [fix] add `hasShouldComponentUpdateBug` to gDSFP lifecycles option (#1981)
 - [deps] update `react-is`, `enzyme-adapter-utils`, `prop-types`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
API: shallow package: react adapter: 16 package: react adapter: 16.3 semver: minor New stuff. semver: patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@chenesan @ljharb