@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

Generated by 🚫 dangerJS

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.

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.

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

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.

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

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.

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

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.

Thanks for explaining!

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

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

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.

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

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.

bump

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!