Add contextType support - #2554
Conversation
Add support for passing context to React class based components that request context via setting .contextType, according to patches posted in enzymejs#2189 (comment). Adds changes to ReactSixteenAdapter and simple test case. Co-authored-by: Kevin Read <me@kevin-read.com> Co-authored-by: Pablo Palacios <pablo.palacios@holidaycheck.com>
|
@pablopalacios thanks, but opening an additional PR is hugely disruptive - now both PRs must stay open forever, manually kept in sync, until both can be landed. In the future, please post a link to your branch on the original PR, and I can pull in your changes. |
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
- #2507 (comment) (same changes needed in the 16.3 adapter)
- #2507 (review) (add a test that changes the provided value, and ensures that the new value also shows up)
| const context = getMaskedContext(Component.contextTypes, unmaskedContext); | ||
| let context; | ||
| if (Component.contextType) { | ||
| const Provider = adapter.getProviderFromConsumer(Component.contextType); |
There was a problem hiding this comment.
what happens in React when .contextType isn't set to a proper Consumer?
There was a problem hiding this comment.
What do you mean with a proper consumer? A consumer without a parent provider? It should display the default provider value.
There was a problem hiding this comment.
like, for example, what if Component.contextType is a random value - some object, or function, or primitive? What exact check does React do?
|
I'll take a closer look at test failures when I have time to do so. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2554 +/- ##
===========================================
- Coverage 96.42% 71.97% -24.46%
===========================================
Files 49 49
Lines 4332 4210 -122
Branches 1156 1132 -24
===========================================
- Hits 4177 3030 -1147
- Misses 155 1180 +1025
... and 29 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
092f6d4 to
3344a07
Compare
|
@ljharb I can close this PR and post the branch in the other one, no problem. Regarding applying the changes in the 16.3 adapter: does it make sense? React just added support for |
|
Please don't; once a PR is opened its a permanent ref on the repo, so it needs to stay open until it's merged. |
|
hmm, if contextType is only added in 16.6 then you’re right, it should only go in the 16 adapter |
3344a07 to
583a764
Compare
583a764 to
3b8aab4
Compare
|
@ljharb I finally had time to take a look a this one again. I was able to make the missing test to pass. Could you review it again? |
43eb75e to
39e6b1f
Compare
3b8aab4 to
16cf05f
Compare
16cf05f to
658422b
Compare
|
@pablopalacios i rebased everything and made a few fixes; looks like tests are failing on react ^16.6. |
|
gotcha, thanks, i'll take a look at that one. |
|
@pablopalacios actually enzyme doesn't use |
|
@ljharb it depends on |
|
We use v16 of react-test-renderer which does not depend on that - only v17+ does. |
3 participants
@ljharb I've rebased #2507 , applied your suggestions and added test cases for dynamic context values. Unfortunately, the test which uses
wrappingComponentdoes not pass, it keeps rendering the initial context value. The context is correctly updated in theRootFinderthough, but it's not propagated down the tree.I tried my best to understand what is happening but I don't understand yet how things work internally yet. If you have any hint to point me in the right direction, I'll gladly move this PR forward.
Thank you for all the great work :)