Add support for react context element types, fixes #1509#1513
Add support for react context element types, fixes #1509#1513aweary merged 7 commits intoenzymejs:masterfrom
Conversation
|
This might be better as a new adapter like |
|
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 |
|
Yeah, I think reducing churn is good concern. There is precedence for creating a new adapter for a minor release ( |
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 |
|
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? |
|
I think there are two concerns here. The first, being enzyme should work in react trees that use
So this isn't the case, if a user renders a Provider/Consumer it'll work, and internally if passed Ultimately tho I think Enzyme even needs a |
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 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 const theme = {
button: { fontSize: 15 }
}
const root = mount(<Button label="foo">, { context: { theme } })
The one thing that sucks about requiring that consumers always exist inside a provider is that with |
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 That's why |
|
Phew! :P
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 |
| "eslint-plugin-react": "^7.5.1" | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
please ensure all files always have a trailing newline
| case ContextProvider: | ||
| return toTree(node.child); | ||
| case ContextConsumer: | ||
| return toTree(node.child); |
There was a problem hiding this comment.
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.
| "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" |
There was a problem hiding this comment.
Why do we need to do this?
There was a problem hiding this comment.
^16.0.0-0 only handles prerelease ranges the exact version 16.0.0 not any prerelease of any version of 16
|
Hi there! Any news on that? |
|
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
left a comment
There was a problem hiding this comment.
Could we also add tests for shallow?
| return node.memoizedProps; | ||
| case Fragment: // 10 | ||
| case Mode: // 11 | ||
| case ContextProvider: // 13 |
There was a problem hiding this comment.
O.o ContextProvider and ContextConsumer are new node types, they're not just components?
|
Thank you! |
|
@jquense - Enzyme's old style context isn't working for me with React 16's new Context... This is the code I have... package.json Error.js Error.test.js The context never gets set for me. Is the above correct? @belfz - did the old style context work for you when using React 16's new Context? |
|
@james-ga11agher that won't work you can't use old style context with new context. they aren't compatible apis. |
|
@jquense - sorry, not following, "you can use old style context with new context. they aren't compatible apis." Is this not what I'm doing, using Enzyme's old style of setting context with React's new Context? Maybe I'm misunderstanding? Does Enzyme support React 16's new Context? Is there an example of this? On my search for this, this was the most promising posting. |
|
The Enzyme API's for setting context are only for old style context, they don't work for the new context api. if you want to set context for a tree using the new context API, render the Provider as part of the test. e.g. |
|
Ah I get you now, thank you so much 👍 Really appreciate the help 👍 In case anyone else has the same issue as me, working code below |
|
@james-ga11agher no, enzyme does not yet support new-style context, see #1553. This PR is only one piece of it. |
|
Following up on the above comments about the existing 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. 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 |
|
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 :-) |
|
@ljharb Oh, I thought the new context support was mostly done already (I was looking at #1553, where only the issue about Regarding |
|
|
|
Is this now part of a release? |
|
@c10b10 yes. if you're still seeing an issue, please file one. |
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