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

Allow .contextType on class based components #100

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

unverbraucht
Copy link

Hi all,

the patch posted on enzymejs/enzyme#2189 adds support for class based components that pass the context via .contextType. This is the part of the patch that applies to react-shallow-renderer.

Copy link

@dcpesses dcpesses left a comment

Choose a reason for hiding this comment

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

@unverbraucht shouldn't it be checking for elementType.contextType instead of element.contextType?

Added line-specific comment

@@ -500,7 +500,9 @@ See https://fb.me/react-invalid-hook-call for tips about how to debug and fix th

this._rendering = true;
this._element = element;
this._context = getMaskedContext(elementType.contextTypes, context);
this._context = element.contextType
Copy link

Choose a reason for hiding this comment

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

@unverbraucht shouldn't it be checking for elementType.contextType instead of element.contextType?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure, the approach in the PR worked for me. I'm afraid I can't test it anymore, we moved away from react-shallow-renderer completely on the project.

Choose a reason for hiding this comment

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

Fair point if you've already moved on, which is what I'm already planning to do. However, I just confirmed that, while element.contextType is not defined, elementType.contextType is. Hopefully this little tidbit will help someone, even if this PR is never updated and merged.

Choose a reason for hiding this comment

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

@ljharb I'm willing to update this PR since it's a blocker for enzymejs/enzyme#2554. Operational wise, how should we do it? A branch on my fork? Another PR?

Copy link
Member

Choose a reason for hiding this comment

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

@pablopalacios please post a link to a branch on your fork - NOT a separate PR - and i can pull its changes into this PR. Thanks!

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@unverbraucht @dcpesses after adding some tests, I tried both versions. elementType is the right one (and the one that makes sense since we extract the legacy context from it as well).

Co-authored-by: Kevin Read <me@kevin-read.com>
Co-authored-by: Pablo Palacios <pablo.palacios@holidaycheck.com>
@ljharb
Copy link
Member

ljharb commented Oct 7, 2022

@pablopalacios i've updated the PR, but there's still no tests or anything.

@pablopalacios
Copy link

@ljharb I've updated my branch with the tests. I also renamed the tests that are using the legacy contextTypes property.

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

Successfully merging this pull request may close these issues.

4 participants