[FIX] Allow shallow renderer to work with new API #14329

mprobber · 2018-11-26T17:51:19Z

Before submitting a pull request, please make sure the following is done:

Fork the repository and create your branch from master.
Run yarn in the repository root.
If you've fixed a bug or added code that should be tested, add tests!
Ensure the test suite passes (yarn test). Tip: yarn test --watch TestName is helpful in development.
Run yarn test-prod to test in the production environment. It supports the same options as yarn test.
If you need a debugger, run yarn debug-test --watch TestName, open chrome://inspect, and press "Inspect".
Format your code with prettier (yarn prettier).
Make sure your code lints (yarn lint). Tip: yarn linc to only check changed files.
Run the Flow typechecks (yarn flow).
If you haven't already, complete the CLA.

Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html

Hi,
I really like the new contextType API in 16.6, but the shallow renderer doesn't seem to provide support for it. This pull request allows you to pass in an object for context, and if contextType is defined, will shallow render the component with the provided context, as it did for the old context API.

gaearon · 2018-11-28T18:58:35Z

packages/react-test-renderer/src/ReactShallowRenderer.js

@@ -143,7 +143,12 @@ class ReactShallowRenderer {

    this._rendering = true;
    this._element = element;
-    this._context = getMaskedContext(element.type.contextTypes, context);
+
+    if (element.type.contextType) {


This should only be enabled for class components.

gaearon · 2018-11-28T18:59:25Z

packages/react-test-renderer/src/ReactShallowRenderer.js

-    this._context = getMaskedContext(element.type.contextTypes, context);
+
+    if (element.type.contextType) {
+      this._context = context;


This is the legacy context object. It has no relation to the new context API you're implementing.

gaearon

This doesn't implement the new context API. Your test should verify that this.context is equal to 'hello world' (because it's the default context value). I also don't think we want to support passing second value to shallowRenderer.render for new context because which context component subscribes to is an implementation detail. Maybe we should support rendering <Provider value={...}><Thing /></Provider> though. Not sure.

mprobber · 2018-11-28T20:08:09Z

@gaearon

I agree with you that the shallow renderer should use the value of the default context if no context is provided, but I'm not really sure about rendering two levels deep and not being transparent about it in the shallow renderer for a select few components.

Looking at

react/packages/react-reconciler/src/ReactFiberClassComponent.js

Line 362 in c954efa

warningWithoutStack(

there is no way for a component to support both the contextType and the legacy contextTypes APIs, so it doesn't seem like it would be problematic to use the same argument to set context on a component (just an object, used to set this.context). I don't have too much context (ba-dum-tss) about implementation details though, so I'm curious if there's something I'm missing. I'll update the PR so the default context renders if none is provided.

sizebot · 2018-11-28T21:22:01Z

Details of bundled changes.

Comparing: 409066a...1f656b3

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	0.0%	0.0%	443.03 KB	443.03 KB	95.65 KB	95.65 KB	UMD_DEV
react-test-renderer.production.min.js	0.0%	0.0%	56.15 KB	56.15 KB	17.23 KB	17.23 KB	UMD_PROD
react-test-renderer.development.js	0.0%	0.0%	438.24 KB	438.24 KB	94.5 KB	94.5 KB	NODE_DEV
react-test-renderer.production.min.js	0.0%	0.0%	55.82 KB	55.82 KB	17.08 KB	17.08 KB	NODE_PROD
ReactTestRenderer-dev.js	0.0%	0.0%	446.12 KB	446.12 KB	93.72 KB	93.72 KB	FB_WWW_DEV
react-test-renderer-shallow.development.js	+1.5%	+1.0%	25.79 KB	26.17 KB	7.04 KB	7.11 KB	UMD_DEV
react-test-renderer-shallow.production.min.js	🔺+1.1%	🔺+1.4%	7.28 KB	7.36 KB	2.38 KB	2.41 KB	UMD_PROD
react-test-renderer-shallow.development.js	+1.9%	+1.3%	20.18 KB	20.56 KB	5.61 KB	5.68 KB	NODE_DEV
react-test-renderer-shallow.production.min.js	🔺+1.0%	🔺+1.0%	7.96 KB	8.04 KB	2.64 KB	2.67 KB	NODE_PROD
ReactShallowRenderer-dev.js	+2.1%	+1.5%	18.13 KB	18.51 KB	4.75 KB	4.82 KB	FB_WWW_DEV

Generated by 🚫 dangerJS

jfirebaugh · 2019-01-31T20:00:12Z

It doesn't seem like it would be problematic to use the same argument to set context on a component (just an object, used to set this.context).

Coming here from enzymejs/enzyme#1553, via https://stackoverflow.com/questions/53711166/unable-to-set-context-in-an-enzyme-test, and that's pretty much exactly how I expected it to work.

minznerjosh · 2019-02-07T15:33:24Z

To @gaearon's point, using the second argument to support contextType would create some funkiness in enzyme.

It seems reasonable at first because, with the plain react shallow renderer, you're only ever rendering one component at a time, so you have a lot of control over what context you pass when you render a component; you know not to pass legacy context if the component you're testing is using contextType.

But Enzyme has the concept of dive()ing, allowing you to render components further down in the tree. And, most importantly, when you dive() a component, the legacy context of any providers further up in the tree will be passed as context (via the second argument to .render()).

So, if this change were to be released, and no changes in Enzyme were made, if you were to dive() a component that uses contextType you would end up with a bunch of legacy context as this.context in your component.

Now, of course, Enzyme could be updated to inspect the component being dive()d to determine whether new or legacy context should be passed. But I still think it's worth calling out that this change is a little more "break-y" then it appears at first. And I think this potentially odd behavior is a decent smell for the fact that new context and legacy context are actually totally different APIs, and we maybe shouldn't overload the second arg to .render() to handle both.

mprobber · 2019-02-08T01:40:37Z

@minznerjosh

The only thing coming to mind to that would avoid overloading the second argument, and supporting contextType with that use case would be we passing a Map of React.Context, value pairs, but that seems very, very clunky.

I'd think that a follow-up change should be made to enzyme regardless if this PR gets merged. Currently, if you shallow render a component with contextType, this.context is the empty object, as opposed to the default value of the context. So, while this would make dive() behave bizarrely for components that use contextType, they already behaves incorrectly under enzyme. Regardless, I'd be happy to craft a pull request for enzyme when any changes are made.

Let me know if you come up with an API that doesn't overload the second argument to render that's not super clunky. I'd be happy to implement it.

gaearon · 2019-03-15T15:13:14Z

But Enzyme has the concept of dive()ing

Oh my. This is the first time I'm hearing and reading about this API 😳
This seems like a real can of worms.

ljharb · 2019-03-15T15:47:30Z

.dive() is just sugar for a feature enzyme has always had since v1, the ability to find an arbitrary custom component and create a new shallow wrapper from that point.

gaearon · 2019-03-15T19:32:18Z

The confusing thing to me is that the updates in the parent component wouldn't "reflect" in that "dived" version for shallow. But they would for mount. Unless I'm missing something.

minznerjosh · 2019-03-15T19:37:13Z

@gaearon I'm pretty sure you're correct about this. If you change state/props in the parent, you'd need to re-dive to see the changes reflected in the "dived" version. This, however, probably isn't super surprising to users given the immutable nature of non-root enzyme wrappers.

However, it does present itself if you ever want to test component lifecycle methods in the dived child by manipulating the props/state of the parent.

ljharb · 2019-03-15T19:45:08Z

That's also the case for both shallow and mount for any derived wrapper - ie, any .find-produced wrapper is not updated if the parent updates, you always have to re-find from the root.

gaearon · 2019-03-15T21:23:45Z

Thanks for explaining!

DavyJohnes · 2019-04-22T12:23:39Z

Guys, is there any workaround to pass context to component which uses new context API?

d4wei · 2019-06-07T17:27:01Z

@mprobber @gaearon Any chance we might move forward with this, or decide on an alternative API in the near future? Having shallow rendering support the new contextType API would be really useful!

If we are thinking about alternatives, would passing a map of Provider/value pairs when creating an instance of shallowRenderer(instead of passing it when render is called) be a less clunky way to do this while avoiding the problems @minznerjosh described? I think in a certain sense, non-default values for contexts can also be interpreted as "configurations" for a shallowRenderer instance.

Guys, is there any workaround to pass context to component which uses new context API?

@DaveVoyles If the context your component uses happens to be an object, you can use the legacy API to test it for now by setting an appropriate value for contextTypes on your component at test time. I've documented this workaround in the Stack Overflow question that led me here.

comp615 · 2019-08-30T20:53:08Z

Wanted to throw my hat into the ring too, having a solution here would greatly help unblocking our upgrade path to remove old-style, legacy context. Right now we code-modded most of it away and the code is functioning, but tests are an absolute mess. Can explore the workaround detailed above, but that feels a bit worse.

mjancarik · 2019-12-13T18:56:46Z

I created module for workaround https://www.npmjs.com/package/shallow-with-context. The module works well in our projects.

stale · 2020-03-12T19:40:33Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

ljharb · 2020-03-12T20:18:05Z

bump

gaearon · 2020-03-14T13:35:50Z

The shallow renderer has been extracted so please port this change to https://github.com/NMinhNguyen/react-shallow-renderer if you'd like. Thank you!

Allow shallow renderer to work with new API

072294a

facebook-github-bot added the CLA Signed label Nov 26, 2018

Revert yarn lockfile, as package is on an older version

1e2b714

This was referenced Nov 26, 2018

Shallow API setContext should work without static contextTypes for React 16 enzymejs/enzyme#1913

Open

Complete React 16 support enzymejs/enzyme#1553

Open

gaearon reviewed Nov 28, 2018

View reviewed changes

gaearon requested changes Nov 28, 2018

View reviewed changes

Use default value for contextType context API

1f656b3

oliviertassinari mentioned this pull request Apr 21, 2019

Shallow testing (enzyme) component with custom theme and mui-styles' makeStyles hook mui/material-ui#15404

Closed

jquense mentioned this pull request Aug 12, 2019

proposal: implement components use the new hook formatjs/formatjs#1290

Closed

stale bot added the Resolution: Stale Automatically closed due to inactivity label Mar 12, 2020

stale bot removed the Resolution: Stale Automatically closed due to inactivity label Mar 12, 2020

gaearon closed this Mar 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Allow shallow renderer to work with new API #14329

[FIX] Allow shallow renderer to work with new API #14329

mprobber commented Nov 26, 2018

gaearon Nov 28, 2018

gaearon Nov 28, 2018

gaearon left a comment

mprobber commented Nov 28, 2018

sizebot commented Nov 28, 2018

react-test-renderer

jfirebaugh commented Jan 31, 2019

minznerjosh commented Feb 7, 2019

mprobber commented Feb 8, 2019 •

edited

Loading

gaearon commented Mar 15, 2019 •

edited

Loading

ljharb commented Mar 15, 2019

gaearon commented Mar 15, 2019

minznerjosh commented Mar 15, 2019

ljharb commented Mar 15, 2019

gaearon commented Mar 15, 2019

DavyJohnes commented Apr 22, 2019

d4wei commented Jun 7, 2019

comp615 commented Aug 30, 2019

mjancarik commented Dec 13, 2019

stale bot commented Mar 12, 2020

ljharb commented Mar 12, 2020

gaearon commented Mar 14, 2020

[FIX] Allow shallow renderer to work with new API #14329

[FIX] Allow shallow renderer to work with new API #14329

Conversation

mprobber commented Nov 26, 2018

gaearon Nov 28, 2018

Choose a reason for hiding this comment

gaearon Nov 28, 2018

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

mprobber commented Nov 28, 2018

sizebot commented Nov 28, 2018

react-test-renderer

jfirebaugh commented Jan 31, 2019

minznerjosh commented Feb 7, 2019

mprobber commented Feb 8, 2019 • edited Loading

gaearon commented Mar 15, 2019 • edited Loading

ljharb commented Mar 15, 2019

gaearon commented Mar 15, 2019

minznerjosh commented Mar 15, 2019

ljharb commented Mar 15, 2019

gaearon commented Mar 15, 2019

DavyJohnes commented Apr 22, 2019

d4wei commented Jun 7, 2019

comp615 commented Aug 30, 2019

mjancarik commented Dec 13, 2019

stale bot commented Mar 12, 2020

ljharb commented Mar 12, 2020

gaearon commented Mar 14, 2020

mprobber commented Feb 8, 2019 •

edited

Loading

gaearon commented Mar 15, 2019 •

edited

Loading