Add support for react context element types, fixes #1509

[jquense]

There are some outstanding support concerns here with shallow rendered per facebook/react#12152 but i wasn't attempting to fix those here. This just adds support for traversal over said elements. I was not sure how to structure the peer deps in a way that would exercise this test as well...

cc @aweary

[aweary]

This might be better as a new adapter like ReactSixteenThreeAdapter, sincem 16.2 and earlier won't have the new context API. It won't hurt to check for ContextProvider and ContextConsumer in earlier React 16 versions, but the advantage of the adapter pattern is that we can keep version-specific stuff isolated. It will possibly depend on how context is supported in the new shallow renderer as well.

[jquense]

my thought was that it'd reduce some churn for folks since it's not a breaking change but that might go out the window for handling context only using hte new API. In which case it may be easier to make a new one...Idk

[aweary]

Yeah, I think reducing churn is good concern. There is precedence for creating a new adapter for a minor release (ReactFifteenFourAdapter), but to be fair that split was around before we required people to explicitly configure an adapter. I just think if we're going to support traversing the new context elements, we should also support actually using the new context API, so it might be best to hold off until we figure out what that'll look like.

[jquense]

we should also support actually using the new context API, so it might be best to hold off until we figure out what that'll look like.

definately agree on supporting hte new context, but most of that change is gonna be internal ReactWrapper stuff. It'd be nice to at least not have tests break for folks who start using the context api in their own code. I mean if we can do it at the same time great, but i'm not confident in my ability to tackle it so idk

[aweary]

I guess my concern is that if context doesn't actually work at all, maybe it should break? I don't want anyone to assume because it renders the providers/consumers without failing that that means the adapter supports them. I also think it should fail if you try to use it with React <=16.2, which is why I'm leaning towards a separate adapter. We didn't do that with fragments though AFAIK, so maybe it's not a big deal.

I haven't been paying a lot of attention to what's been happening with Enzyme, @ljharb can you chime in here?

[jquense]

I think there are two concerns here. The first, being enzyme should work in react trees that use createContext, and the second is that internally Enzyme mount() and shallow should handle their context options with the new APi since it'll break when the old on is removed.

I guess my concern is that if context doesn't actually work at all

So this isn't the case, if a user renders a Provider/Consumer it'll work, and internally if passed context it'll also work but use the old api.

Ultimately tho I think Enzyme even needs a context option (depending on shallow renderer) since its straightforward to do in a test manually now, e.g. <Provider><Root></Provider> that and there is probably no way to make the enzyme api work without a breaking change since you can't arbitrarily pass context anymore, you must have the paired Consumer to access it.

[aweary]

I think there are two concerns here. The first, being enzyme should work in react trees that use createContext, and the second is that internally Enzyme mount() and shallow should handle their context options with the new API since it'll break when the old on is removed.

Right, I'm suggesting that if one works the other should probably work as well. Mostly because people will probably be rendering a lot of consumers without providers.

So this isn't the case, if a user renders a Provider/Consumer it'll work, and internally if passed context it'll also work but use the old api.

So my worry is related to the second concern you mentioned, specifically users thinking that Enzyme's API for providing context to a tree will work rendering standalone consumers. I'm assuming this would not work:

const Button = ({ label }) => (
  <ThemeConsumer>
    {theme => <button style={theme.button}>{label}</button>}
  </ThemeConsumer>
)

and they try to pass it context via mount's option like:

const theme = {
  button: { fontSize: 15 }
}
const root = mount(<Button label="foo">, { context:  { theme }  })

Ultimately tho I think Enzyme even needs a context option (depending on shallow renderer) since its straightforward to do in a test manually now, e.g. that and there is probably no way to make the enzyme api work without a breaking change since you can't arbitrarily pass context anymore, you must have the paired Consumer to access it.

The one thing that sucks about requiring that consumers always exist inside a provider is that with shallow you'll always have to use dive to get to the actual rendered component(s). It's the same problem we have now with shallow rendering trees with react-redux providers and other similar APIs, but at least with the existing context API you can side-step the issue and pass the store or whatever into the context option.

[jquense]

The one thing that sucks about requiring that consumers always exist inside a provider

is this the case? That will break so much of my context code...

[aweary]

is this the case? That will break so much of my context code...

Not in the general case. It will just use the default value provided to createContext. I mean in the context (no pun intended 😄) of testing components in isolation; you'll probably want to test that Button with something other than the default value in some cases, and requiring a Provider to do that might not be the best solution.

That's why mount and shallow have the context option, so you don't have to wrap your components in actual context providers.

[jquense]

Phew! :P

That's why mount and shallow have the context option, so you don't have to wrap your components in actual context providers.

definitely, I was thinking though that the new API can't work like this, the user will at the least have to provide Enzyme with the provider since it can't take a plain object and send to the correct consumers. AT the very least the API will change to passing a Provider to context: in which case i might be simpler to just have the user use the provider.

[ljharb]

please ensure all files always have a trailing newline

[aweary]

Looks like the test renderer treats them as fragments facebook/react#12237 Should we do the same? It's really confusing how both files use the same APIs (toTree, childrenToTree) but don't use them consistently.

[aweary]

Why do we need to do this?

[jquense]

^16.0.0-0 only handles prerelease ranges the exact version 16.0.0 not any prerelease of any version of 16

[stepankuzmin]

Hi there! Any news on that?

[jquense]

I updated, let me know if there is anything i can do here :) I'd love to see this in a release, we're already playing with react 16.3.0 and the context api and it's killing our tests :P

[ljharb]

Could we also add tests for shallow?

[ljharb]

O.o ContextProvider and ContextConsumer are new node types, they're not just components?

[mbrowne]

Following up on the above comments about the existing context option in mount() and shallow(), I'm wondering if it will ever be possible to achieve a similar result using the new context API. I am currently in the process of setting up a theme for the styled-components in the app I'm working on. This is requiring updates to almost every test case that uses mount(), because either the component directly uses styled-components that need theme properties, or it uses the theme indirectly via a separate npm package with our shared UI components (also styled-components). So for every test where we could previously do something like this:

    const component = mount(<CreatorSummary creator={mockCreator} />)
    ...
    expect(component.state().open).toBe(true)

We would now have to do this:

    const component = mount(<ThemeProvider theme={theme}><CreatorSummary creator={mockCreator} /></ThemeProvider>)
    ...
    expect(component.find(CreatorSummary).instance().state.open).toBe(true)

...and this is just the tip of the iceburg -- the tests are already written so there is a lot of such code that would need to be updated.

With the current API (which uses the legacy context), I was able to set up helper functions, e.g. mountWithTheme(), that use enzyme's context and childContextTypes options, as explained here - which is obviously a much more convenient solution, and IMO a more elegant one too.

But soon we will be upgrading to styled-components 4 (currently in beta), which uses the new context API. So far the best solution I've been able to come up with for the new version is this hack that overrides one of styled-component's internal methods, which obviously isn't ideal:

...
import * as theme from './theme'

function setupMockTheme() {
    const result = enzyme.mount(React.createElement(styled.div``))
    const BaseStyledComponent = result.childAt(0).type()

    BaseStyledComponent.prototype.renderOuter = function renderOuter(styleSheet) {
        this.styleSheet = styleSheet
        return this.renderInner(theme)
    }
}

Would it somehow be possible to add an option to enzyme so context properties could still be passed as an option to mount() and shallow() in a way that would work with the new context API? I realize the new context API works quite differently...any thoughts or suggestions are welcome. As an alternative, it might be helpful if it were possible to call methods like state() on non-root nodes (so tests wrapped with Provider components could still be written in a similar way), but that seems to be an inherent and deliberate limitation of enzyme...

[ljharb]

See #1802, for setting state.

It may indeed be possible; i can’t know until i build support for new context.

My advice would be not to update to something that used new context until you’re able to test it properly :-)

[mbrowne]

@ljharb Oh, I thought the new context support was mostly done already (I was looking at #1553, where only the issue about dive() not working is still open). Good to know there will be more robust support for it in the future.

Regarding state() and setState() and #1802, this is great. Are there any plans to allow other methods like props(), prop(), and setProps() to work on child nodes as well?

[ljharb]

props and prop should already work on anything; setProps is a bit different since the child's parent controls what props it gets - i'm not sure it makes sense to allow that. Either way, let's discuss this in a new issue, since it's all off topic for this PR.

[c10b10]

Is this now part of a release?

[ljharb]

@c10b10 yes. if you're still seeing an issue, please file one.