Skip to content

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

Merged
ljharb merged 4 commits into
enzymejs:masterfrom
chenesan:test-gDSFP-before-sCP
Mar 12, 2019
Merged

[Fix] shallow: ensure SCU is called after GDSFP returns null#1981
ljharb merged 4 commits into
enzymejs:masterfrom
chenesan:test-gDSFP-before-sCP

Conversation

@chenesan

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

Copy link
Copy Markdown
Contributor

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

Fixes #1970.

@ljharb

ljharb commented Jan 19, 2019

Copy link
Copy Markdown
Member

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

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

chenesan commented Feb 7, 2019

Copy link
Copy Markdown
Contributor Author

@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

ljharb commented Feb 7, 2019

Copy link
Copy Markdown
Member

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

@ljharb
ljharb force-pushed the test-gDSFP-before-sCP branch from 926b306 to 8e7c84a Compare February 7, 2019 07:38
@ljharb

ljharb commented Feb 7, 2019

Copy link
Copy Markdown
Member

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

chenesan commented Feb 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@ljharb I think it's not fixable in 16.3 because it's fixed in [email protected]. 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

chenesan commented Feb 8, 2019

Copy link
Copy Markdown
Contributor Author

@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

ljharb commented Feb 8, 2019

Copy link
Copy Markdown
Member

@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

chenesan commented Feb 9, 2019

Copy link
Copy Markdown
Contributor Author

@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

ljharb commented Feb 9, 2019

Copy link
Copy Markdown
Member

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

chenesan commented Feb 28, 2019

Copy link
Copy Markdown
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
ljharb force-pushed the test-gDSFP-before-sCP branch from 9e2b05e to faea638 Compare March 11, 2019 07:35
@ljharb

ljharb commented Mar 11, 2019

Copy link
Copy Markdown
Member

I've updated/rebased this; but it seems like react/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.

@ljharb
ljharb force-pushed the test-gDSFP-before-sCP branch from faea638 to b66ac28 Compare March 11, 2019 07:47
@chenesan

chenesan commented Mar 12, 2019

Copy link
Copy Markdown
Contributor Author

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

@ljharb
ljharb force-pushed the test-gDSFP-before-sCP branch from b66ac28 to 504e4fc Compare March 12, 2019 05:01
@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

ljharb commented Mar 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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 test-gDSFP-before-sCP branch from 504e4fc to 98154a9 Compare March 12, 2019 06:28
@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.

2 participants

@chenesan @ljharb