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

Add support for react context element types, fixes #1509 #1513

Merged
merged 7 commits into from
Mar 30, 2018

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Feb 5, 2018

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
Copy link
Collaborator

aweary commented Feb 5, 2018

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
Copy link
Contributor Author

jquense commented Feb 5, 2018

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
Copy link
Collaborator

aweary commented Feb 5, 2018

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
Copy link
Contributor Author

jquense commented Feb 5, 2018

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
Copy link
Collaborator

aweary commented Feb 5, 2018

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
Copy link
Contributor Author

jquense commented Feb 5, 2018

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
Copy link
Collaborator

aweary commented Feb 5, 2018

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
Copy link
Contributor Author

jquense commented Feb 5, 2018

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
Copy link
Collaborator

aweary commented Feb 5, 2018

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
Copy link
Contributor Author

jquense commented Feb 5, 2018

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.

@@ -49,4 +49,4 @@
"eslint-plugin-jsx-a11y": "^6.0.3",
"eslint-plugin-react": "^7.5.1"
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

please ensure all files always have a trailing newline

case ContextProvider:
return toTree(node.child);
case ContextConsumer:
return toTree(node.child);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@@ -40,12 +40,12 @@
"object.values": "^1.0.4",
"prop-types": "^15.6.0",
"react-reconciler": "^0.7.0",
"react-test-renderer": "^16.0.0-0"
"react-test-renderer": "^16.0.0-0 || ^16.3.0-0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@stepankuzmin
Copy link

Hi there! Any news on that?

@jquense
Copy link
Contributor Author

jquense commented Mar 5, 2018

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Could we also add tests for shallow?

@@ -111,6 +112,11 @@ function toTree(vnode) {
}
case HostText: // 6
return node.memoizedProps;
case Fragment: // 10
case Mode: // 11
case ContextProvider: // 13
Copy link
Member

Choose a reason for hiding this comment

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

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

@ljharb
Copy link
Member

ljharb commented Sep 9, 2018

@james-ga11agher no, enzyme does not yet support new-style context, see #1553. This PR is only one piece of it.

@mbrowne
Copy link

mbrowne commented Sep 13, 2018

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

ljharb commented Sep 14, 2018

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

mbrowne commented Sep 14, 2018

@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
Copy link
Member

ljharb commented Sep 14, 2018

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

c10b10 commented Jan 18, 2019

Is this now part of a release?

@ljharb
Copy link
Member

ljharb commented Jan 18, 2019

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

devs-cloud pushed a commit to devs-cloud/react-ui-router that referenced this pull request Dec 27, 2019
Enzyme does not support React 16.3 context API.
Patch the package until the package is realsed with the fix.
enzymejs/enzyme#1513
rockstar-0000 added a commit to rockstar-0000/react-simple-ui that referenced this pull request Jul 20, 2021
Enzyme does not support React 16.3 context API.
Patch the package until the package is realsed with the fix.
enzymejs/enzyme#1513
Development-KasperSky added a commit to Development-KasperSky/react that referenced this pull request Nov 11, 2022
Enzyme does not support React 16.3 context API.
Patch the package until the package is realsed with the fix.
enzymejs/enzyme#1513
keeneyetact added a commit to keeneyetact/reactjs that referenced this pull request May 8, 2023
Enzyme does not support React 16.3 context API.
Patch the package until the package is realsed with the fix.
enzymejs/enzyme#1513
taeul pushed a commit to taeul/React that referenced this pull request Mar 1, 2024
Enzyme does not support React 16.3 context API.
Patch the package until the package is realsed with the fix.
enzymejs/enzyme#1513
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.