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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Mar 12, 2019

Conversation

Projects
None yet
2 participants
@chenesan
Copy link
Contributor

chenesan commented Jan 18, 2019

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

This comment has been minimized.

Copy link
Member

ljharb commented Jan 19, 2019

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

@chenesan chenesan force-pushed the chenesan:test-gDSFP-before-sCP branch from 070a708 to 926b306 Feb 7, 2019

@chenesan

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

ljharb commented Feb 7, 2019

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

@ljharb ljharb force-pushed the chenesan:test-gDSFP-before-sCP branch from 926b306 to 8e7c84a Feb 7, 2019

@ljharb

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Feb 7, 2019

@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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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 chenesan:test-gDSFP-before-sCP branch 2 times, most recently from 552714e to 9e2b05e Feb 28, 2019

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Feb 28, 2019

@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 added some commits Mar 11, 2019

@ljharb ljharb force-pushed the chenesan:test-gDSFP-before-sCP branch from 9e2b05e to faea638 Mar 11, 2019

@ljharb

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Mar 12, 2019

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

@ljharb ljharb force-pushed the chenesan:test-gDSFP-before-sCP branch from b66ac28 to 504e4fc Mar 12, 2019

@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

This comment has been minimized.

Copy link
Member

ljharb commented Mar 12, 2019

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 force-pushed the chenesan:test-gDSFP-before-sCP branch from 504e4fc to 98154a9 Mar 12, 2019

@ljharb

ljharb approved these changes Mar 12, 2019

@ljharb ljharb merged commit 98154a9 into airbnb:master Mar 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

ljharb added a commit that referenced this pull request Mar 13, 2019

[enzyme-adapter-react-16.3] v1.6.2
 - [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

[enzyme-adapter-react-16] v1.11.0
 - [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
You can鈥檛 perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.