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

Add contextType support #2554

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pablopalacios
Copy link

@ljharb I've rebased #2507 , applied your suggestions and added test cases for dynamic context values. Unfortunately, the test which uses wrappingComponent does not pass, it keeps rendering the initial context value. The context is correctly updated in the RootFinder though, 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 :)

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 <[email protected]>
Co-authored-by: Pablo Palacios <[email protected]>
@ljharb
Copy link
Member

ljharb commented Mar 20, 2022

@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.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

const context = getMaskedContext(Component.contextTypes, unmaskedContext);
let context;
if (Component.contextType) {
const Provider = adapter.getProviderFromConsumer(Component.contextType);
Copy link
Member

Choose a reason for hiding this comment

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

what happens in React when .contextType isn't set to a proper Consumer?

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean with a proper consumer? A consumer without a parent provider? It should display the default provider value.

Copy link
Member

Choose a reason for hiding this comment

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

like, for example, what if Component.contextType is a random value - some object, or function, or primitive? What exact check does React do?

packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Mar 20, 2022

I'll take a closer look at test failures when I have time to do so.

@codecov
Copy link

codecov bot commented Mar 20, 2022

Codecov Report

Patch coverage: 100.00% and project coverage change: -24.46 ⚠️

Comparison is base (3a7701c) 96.42% compared to head (092f6d4) 71.97%.

❗ Current head 092f6d4 differs from pull request most recent head 658422b. Consider uploading reports for the commit 658422b to get more accurate results

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     
Impacted Files Coverage Δ
...enzyme-adapter-react-16/src/ReactSixteenAdapter.js 95.49% <100.00%> (-0.32%) ⬇️

... 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pablopalacios
Copy link
Author

@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 contextType in version 16.6 (see: https://reactjs.org/blog/2018/10/23/react-v-16-6.html) or am I missing something?

@ljharb
Copy link
Member

ljharb commented Mar 20, 2022

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.

@ljharb
Copy link
Member

ljharb commented Mar 20, 2022

hmm, if contextType is only added in 16.6 then you’re right, it should only go in the 16 adapter

@pablopalacios pablopalacios marked this pull request as ready for review October 5, 2022 22:23
@pablopalacios
Copy link
Author

@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?

@ljharb
Copy link
Member

ljharb commented Mar 14, 2023

@pablopalacios i rebased everything and made a few fixes; looks like tests are failing on react ^16.6.

@pablopalacios
Copy link
Author

@ljharb the tests are expected to fail since they use a version of react-shallow-renderer that does not have support for contextType. We need to merge this PR first.

@ljharb
Copy link
Member

ljharb commented Mar 16, 2023

gotcha, thanks, i'll take a look at that one.

@ljharb
Copy link
Member

ljharb commented Apr 14, 2023

@pablopalacios actually enzyme doesn't use react-shallow-renderer at all. Is there no way to fix it prior to taking on that dependency?

@pablopalacios
Copy link
Author

pablopalacios commented Apr 14, 2023

@ljharb it depends on react-test-renderer which depends on react-shallow-renderer.

@ljharb
Copy link
Member

ljharb commented Apr 14, 2023

We use v16 of react-test-renderer which does not depend on that - only v17+ does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants