Skip to content

Allow .contextType on class based components#100

Open
unverbraucht wants to merge 2 commits intoenzymejs:masterfrom
unverbraucht:master
Open

Allow .contextType on class based components#100
unverbraucht wants to merge 2 commits intoenzymejs:masterfrom
unverbraucht:master

Conversation

@unverbraucht
Copy link
Copy Markdown

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
Copy Markdown

@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

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

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
Copy Markdown
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.

Copy link
Copy Markdown

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.

Copy link
Copy Markdown

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
Copy Markdown
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!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

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
Copy Markdown
Member

ljharb commented Oct 7, 2022

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

@pablopalacios
Copy link
Copy Markdown

@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