-
-
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
How should enzyme support the createContext() API #1973
Comments
I am experiencing an issue with this as well as it relates to snapshot testing with Redux connected children. The below mock works well
and snapshots look great for general React components
However when the child is a Redux connected component, I get this for a snapshot
switching to using |
Just as an aside, react-redux has already made their change to the new context api as of v6.0.0 which released just last month so consider me as the first of a flood of people that will be running into this issue shortly. :) Downgrading to react-redux v5.1.1 in the meantime. |
Understood; there's already a bit of discussion on reduxjs/react-redux#1161 about it. |
It's perhaps worth noting that version 4 of |
It's been quite some time since the
createContext()
API shipped in react, and I'd love to help out in implementing "full support" for that API in enzyme ASAP. (Before the libraries many of us depend on [react-router, react-redux, etc] switch from using the deprecated, legacy context API to the new, fully-supported API.)However, based on my reading of many
createContext()
-related issues in this repo, I don't have a clear idea of what "full support" forcreateContext()
in enzyme looks like. So I've created this issue so we can hammer it out.Assumptions
I'm walking into this discussion with the following assumptions. Please challenge them if they don't make sense!
shallow()
andmount()
.createContext()
and the legacy context API simultaneously, just like react.Enzyme's Current (Legacy) Context APIs
I've seen some issues (#1913, #1840) that imply that
options.context
should somehow work with thecreateContext()
API in the future. However, as per my assumptions, I don't understand how this API could supportcreateContext()
. For example, how would this work?The only API I could imagine still making sense for
createContext()
iswrapper.context()
. But even then, it would only be for class components that are using thecontextType
feature added inreact@16.6
, as that's the only part of the API that setsthis.context
in a component.An Idea for Moving Forward
The current context APIs
I think we should move forward with the idea that the existing context APIs (
options.context
,.setContext()
, perhaps.context()
) are only for legacy context, and will never have anything to do with thecreateContext()
API. We should update the docs to reflect this immediately.The
createContext()
APII'm of the opinion that enzyme shouldn't actually add any new APIs for
createContext()
.createContext()
is all about just rendering components! It doesn't involve component methods likegetChildContext()
or static properties likecontextTypes
andchildContextTypes
. If enzyme can handle renderingcreateContext()
's<Consumer />
and<Provider />
, it doesn't need to do anything else!For clarification, here's how one would translate the use of the legacy context APIs using
createContext()
!options.context
createContext()
setContext()
Legacy Context
createContext()
The only problem I see with this approach is that, if you must wrap your component in
<Provider />
s to get your context, your component will never be the root, and it is therefore not possible to call.setProps()
on it. To address this, I'm working on #1960.Thanks for reading! Looking forward to discussing!
The text was updated successfully, but these errors were encountered: