-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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 ability to mount()/shallow() a node inside a wrapping component #1960
Conversation
…ootFinder`, `wrapWithWrappingComponent`, `getWrappingComponentMountRenderer`; add `RootFinder`
…ithWrappingComponent`
|
||
- `.setProviders()` can only be used on a wrapper that was initially created with a call to `mount()` | ||
- `.setProviders()` is only supported in React >= 16.3 | ||
- `.setProviders()` requires `enzyme-adapter-react-16@>=1.8` or `enzyme-adapter-react-16.3@>=1.5` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These versions will need to be updated depending on when/if this makes it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove version numbers entirely; there's no need to specify it here.
Adding createContext support in any form is a major effort; I want to set expectations early that this PR may not make it in at all, and that support is required to be identical for both mount and shallow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing react-specific can go in enzyme itself, only in adapters - as such, a new adapter interface for createContext would have to be added to make this work. Additionally, we'd have to retain compatibility between old enzyme and new adapters, new enzyme and old adapters, and new enzyme and new adapters.
|
||
- `.setProviders()` can only be used on a wrapper that was initially created with a call to `mount()` | ||
- `.setProviders()` is only supported in React >= 16.3 | ||
- `.setProviders()` requires `enzyme-adapter-react-16@>=1.8` or `enzyme-adapter-react-16.3@>=1.5` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove version numbers entirely; there's no need to specify it here.
#### Common Gotchas | ||
|
||
- `.setProviders()` can only be used on a wrapper that was initially created with a call to `mount()` | ||
- `.setProviders()` is only supported in React >= 16.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mount itself has no direct tie to React; it should be able to work as long as the adapter supports it (which might not be react at all)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Is there any prior art for what to do in a case where an adapter does not support a feature? For example, I'm assuming we'll never want to add support for this API to adapters that handle react@<16.3.
reversed.reverse(); | ||
|
||
return reversed.reduce((childElement, parentElement) => ( | ||
React.cloneElement(parentElement, null, childElement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cloning elements is very slow and should be avoided. why is it needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to convert the provider elements the user passes into a tree:
So this:
mount(<MyComponent />, {
providers: [
<ContextA.Provider value="A" />,
<ContextB.Provider value="B" />,
<ContextC.Provider value="C" />,
],
});
Is rendered as:
<ContextA.Provider value="A">
<ContextB.Provider value="B">
<ContextC.Provider value="C">
<MyComponent />
</ContextC.Provider>
</ContextB.Provider>
</ContextA.Provider>
And I figured cloneElement()
was the most idiomatic way to do this. I'm also working under the assumption that mutating the providers the user passes in would be a bad idea. Would a more manual
// not sure if this will actually work
{
...parentElement,
props: {
...parentElement.props,
children: childElement,
},
}
be preferable, or is it the copying itself that is too slow to be acceptable?
|
||
this.setState({ | ||
providers: currentProviders.filter(provider => ( | ||
!providerTypes.includes(provider.type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this API means there's never any way to remove a provider, only to replace existing ones or add new ones.
Separately, providers
being an array implies that the order matters - why and how? If it matters because it's nested into a tree, then it seems much simpler to only ever allow a single provider element, and come up with an alternative solution that lets the user determine their own nesting yet also indicate where children should be rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can effectively remove a provider by doing
wrapper.setProviders([<Context.Provider value={undefined} />]);
As for order mattering, it doesn't. I used an array because objects can only have strings as keys and it seemed wrong to require configuration options that do not have a literal syntax (like Set
or Map
.) I wrote a bit about my thought process when coming up with the API in the PR description.)
But I'm open to an entirely different API if you have some ideas. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused why multiple providers are needed, since you can do:
<Provider1>
<Provider2 />
</Provider1>
and provide a single provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it! I was a bit confused by what you meant by "a single provider". I was thinking about the new context API in terms of individual values (as is the case with the old API), but perhaps that was me trying to make the new API into something that it is not.
The way you compose multiple contexts in the new React API is by nesting components—perhaps the enzyme API should reflect that, as you've pointed out! So, what do you think about this:
mount(<MyComponent />, {
providers: (
<ContextA.Provider value="A">
<ContextB.Provider value="B">
<ContextC.Provider value="C" />
</ContextB.Provider>
</ContextA.Provier>
),
});
And then changing via context by
wrapper.setProviders(
<ContextA.Provider value="A">
<ContextB.Provider value="foo">
<ContextC.Provider value="C" />
</ContextB.Provider>
</ContextA.Provier>
);
My only concern is that this API makes it a bit challenging to change just a single provider's value without having to re-specify the entire tree. I think it will be common to be dealing with a bunch of different contexts from different libraries, all of which are required for a component to render. But each individual spec will deal with the how the component responds to just one of those contexts changing. So, what do you think about (in addition to the .setProviders()
API) having an API like this:
wrapper.updateProvider(<ContextB.Provider value="foo" />);
Which would update just that provider, wherever it is in the tree, and throw an error if the provider is not already in the tree.
Also, I don't feel strongly about any of these names if you'd like them to be different. I am leaning towards keeping the plural "providers" because I think it makes it a bit more clear that the element can have many nested providers, though I do realize you are only passing a single react element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-specifying the entire tree is already the case if you want to change a child of the element you wrapped, for example - i'm also not clear on the use case of changing providers after initially wrapping.
Maybe instead of this feature being specific to createContext
providers, it could be more general - something like, "wrapWith". A single render prop function that gets passed a child, and wraps it as needed.
Example:
const wrapper = mount(<Foo />, {
wrapWith(root) {
return (
<ContextA.Provider value="A">
<ContextB.Provider value="foo">
<ContextC.Provider value="C">
{root}
</ContextC.Provider>
</ContextB.Provider>
</ContextA.Provider>
);
},
});
The API could throw if your wrapWith function failed to render the child (solo, ie, with no siblings), and then wrapper
in this case would be <Foo />
, not any of the providers, but they'd still be exercised in the rendering path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the use-case is something like this:
Imagine you have a react-flag
-like library and some future version of react-redux
that uses the new context API. You'd render your component like this (using the wrapWith API):
// my app's feature flags
const flags = {
foo: false,
bar: false,
};
const wrapper = mount(<Foo />, {
wrapWith: root => (
<FlagProvider value={flags}>
<ReduxProvider value={store}>{root}</ReduxProvider>
</FlagProvider>
),
});
And now we want to test that some side-effect in <Foo />
's componentDidUpdate
gets called when the feature flag changes. We need some way of changing <FlagProvider />
's value
, and it would be nice if we didn't have to re-setup the <ReduxProvider />
when we do that.
Also, if our component starts depending on a new provider in the future, it'd be really nice to only have to add it to the options we pass enzyme, and not every place where we're just changing a single provider's value.
I'm guessing the API for changing the wrapWith
function could be something like
wrapper.setWrapWith(root => (
<FlagProvider value={{ ...flags, bar: true }}>
<ReduxProvider value={store}>{root}</ReduxProvider>
</FlagProvider>
));
But that doesn't address my concern about having to respecify the entire tree. One solution that comes to mind (just spitballing here) is making wrapWith
a component:
const wrapper = mount(<Foo />, {
wrapper: {
component: (props) => {
const { flags, store, children } = props;
return (
<FlagProvider value={{ ...flags, bar: true }}>
<ReduxProvider value={store}>{children}</ReduxProvider>
</FlagProvider>
);
},
props: { store: createStore, flags },
},
});
And then having an API that lets you set the wrapper component's props:
wrapper.setWrapperProps({
flags: {
...flags,
foo: true,
},
});
(wrapWith
seemed like a better name for a function than a component so I changed it to just wrapper
; but it also seems a bit confusing because wrapper
is the conventional name for a ReactWrapper
instance.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We can bikeshed the names later)
OK, so you're suggesting the user be able to provide a "wrapping component", and provide explicit setProps
/setState
access on it - what about instead, wrapper.getWrappingComponent()
, that returned a normal enzyme wrapper around that component - providing you with the same APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea a lot! I'll implement and then drop you a comment here so we can discuss. I really appreciate the time you've taken to work through this with me. Thank you!
@ljharb I figured that'd be the case! Just wanted to get something up here to get a conversation started. |
@ljharb So, I'm sure you've noticed that this PR does not add support for |
02c9040
to
e675ca9
Compare
if (WrappingComponent) { | ||
return ( | ||
<WrappingComponent {...wrappingComponentProps}> | ||
<RootFinder ref={this.setRootFinderInstance}>{component}</RootFinder> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a reliable way to get the node
that was passed as the first argument to mount()
and this seemed like the simplest way. Is it acceptable? Are there going to be performance implications of using a ref
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should avoid using ref
, yes.
@@ -164,6 +164,156 @@ describeWithDOM('mount', () => { | |||
expect(() => wrapper.state('key')).to.throw('ReactWrapper::state("key") requires that `state` not be `null` or `undefined`'); | |||
}); | |||
|
|||
describeIf(is('>= 0.14') ,'wrappingComponent', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason this feature couldn't work in react 13, but the tests don't because react 13 uses owner-based context instead of parent-based. Is it worth coming up with a different test methodology for react 0.13, or are we okay with not supporting this feature in the 0.13 adapter? (I'm not sure how useful this feature is without parent-based context.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we'll solve it for react 13 as well; it's probably OK if we defer for now as long as we're confident this enzyme API won't need to change to support it.
privateSet(this, ROOT, root); | ||
privateSetNodes(this, nodes); | ||
privateSet(this, ROOT_NODES, root[NODES]); | ||
} | ||
privateSet(this, UNRENDERED, nodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UNRENDERED
needs to be set to call .setProps()
on the wrappingComponent
. It didn't seem like there was any harm in setting it to nodes
regardless of !root
. (All the tests still passed.)
packages/enzyme/src/ReactWrapper.js
Outdated
const node = this[WRAPPING_COMPONENT_RENDERER].getNode(); | ||
const wrapper = this.wrap(node); | ||
privateSet(wrapper, ROOT, wrapper); | ||
privateSet(wrapper, RENDERER, this[WRAPPING_COMPONENT_RENDERER]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit hacky. Is it acceptable? We don't want this new ReactWrapper
to render itself, but we do want it to be a root (so we can .setProps()
on it.)
We could also decide to not support .setProps()
and make it a non-root. In that case, we could just get by using .setState()
to change the context.
TL;DR;
I took a stab at implementing the API we discussed for I don't use Perhaps this is because, as others have pointed out, But I still doubt how much sense So, my question is, is there any possibility you'd be open to this being a |
It's a serious issue that |
So, is this the strategy you're looking for in
|
Yep! sounds right to me (where "full render" is actually multiple shallow renders and dives) |
Yup yup. Okay, in that case, I think I'm going to hit the pause button on this PR and try to add support for |
That sounds like an awesome plan! |
34e2c84
to
b26d5f6
Compare
@ljharb updated! |
b26d5f6
to
5c15a18
Compare
Whoops, I can see I broke some stuff. Will fix ASAP! |
ping :-) any update? i'm anxious to get this one in. |
7aaa70b
to
46e9853
Compare
@ljharb updated! The build succeeded! |
46e9853
to
26a34ec
Compare
@ljharb think this could be merged soon? |
26a34ec
to
edf5fc4
Compare
edf5fc4
to
1ca3dc1
Compare
1de841b
to
cc3db79
Compare
- [new] add `isCustomComponent` (#1960) - [dev deps] update `eslint`, `semver`, `rimraf`, `react-is`, `html-element-map`, `chai`, `eslint-plugin-mocha` - [build] include source maps
- [new] add `wrapWithWrappingComponent`, `isCustomComponent` (#1960) - [deps] update `react-is` - [dev deps] update `eslint`, `semver`, `rimraf`, `react-is`, `html-element-map`, `chai`, `eslint-plugin-mocha` - [build] include source maps
- [new] add `wrapWithWrappingComponent`, `isCustomComponent` (#1960) - [deps] update `react-is` - [dev deps] update `eslint` - [build] include source maps
- [new] support shallow rendering `createContext()` providers and consumers - add `isContextConsumer`, `getProviderFromConsumer` (#1966) - [new] add `wrapWithWrappingComponent`, `isCustomComponent` (#1960) - [refactor] use `react-is` predicates more - [deps] update `react-is` - [dev deps] update `eslint` - [build] include source maps
- [new] Add support for wrapping `Profiler` (#2055) - [new] support shallow rendering `createContext()` providers and consumers - add `isContextConsumer`, `getProviderFromConsumer` (#1966) - [new] add `wrapWithWrappingComponent`, `isCustomComponent` (#1960) - [new] add `getDerivedStateFromError` support (#2036) - [fix] avoid invariant violation in provider (#2083) - [fix] properly fix finding memo(SFC) components (#2081) - [fix] properly render memoized SFCs - [fix] `shallow`: avoid wrapping component for hooks - [deps] update `react-is` - [dev deps] update `eslint` - [refactor] use `react-is` predicates more - [build] include source maps
Hi folks, I just ran into this issue and I'm so happy someone's already solved it! From the last comment, does this mean we'll be able to use this feature when the next version of enzyme is released? |
v3.10.0 has now been released. |
Overview
This PR adds an API for
mount()
ing anode
inside of a customwrappingComponent
.It is also possible to get a
ReactWrapper
around thewrappingComponent
:Related to #1958.