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 ability to dive() and shallow-render-as-root Consumers and Providers #1966

Merged
merged 3 commits into from
Apr 4, 2019

Conversation

minznerjosh
Copy link
Contributor

@minznerjosh minznerjosh commented Jan 5, 2019

Fixes #1958
Fixes #1908
Fixes #1647

I think the most controversial choice I've made here is only collecting context when a <Provider /> is the root (either because it is .dive()ed or passed directly to shallow(<Provider />)).

If we wanted to reliably make a <Consumer />'s value be that of the closest <Provider />, we'd need to recursively .dive() from the top until we find that <Provider />. I don't like that idea because we could end up rendering components that the end user did not want rendered.

I think, if the user does want all their <Provider />s' values to be automatically collected, that's where wrappingComponent (from #1960) will come into play.

render(el, context) {
render(el, context, {
providerValues = new Map(),
} = {}) {
Copy link
Contributor Author

@minznerjosh minznerjosh Jan 5, 2019

Choose a reason for hiding this comment

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

I'd like to make the third argument an object. This way we can easily pass additional params as enzyme/react evolves without introducing breaking changes.

@ljharb
Copy link
Member

ljharb commented Jan 7, 2019

enzyme doesn't yet have any support for createContext, so there shouldn't be any mention of "Consumer" or "Provider" in documentation until that's done.

Providers, prior to createContext being a feature, are components that provide child context.

@minznerjosh
Copy link
Contributor Author

I'm a bit confused. There are a couple of tests (this one and this one) that specifically deal with the createContext() API. When mount()ing, the proper value gets passed down and everything—it functions fully! W/r/t shallow(), a component with a <Provider /> or <Consumer /> will render, but you currently can't .dive() it to make it render for real. So it seems like enzyme does have some support for createContext(). Once you are able to .dive() <Consumer />s and <Provider />s, as this PR implements, what more must enzyme do to support .createContext()?

I do think the gotcha about needing to .dive() a <Provider /> to have it affect a downstream <Consumer /> does need to be called out in documentation. Perhaps I could add some clarification that this regards the new createContext() API?

@minznerjosh minznerjosh force-pushed the josh__dive-on-new-context branch from 041cdc0 to d2e3db0 Compare January 8, 2019 13:42
@minznerjosh
Copy link
Contributor Author

@ljharb I've created #1973 to try to come to consensus on what full support for createContext() in enzyme should look like.

@minznerjosh minznerjosh force-pushed the josh__dive-on-new-context branch from d2e3db0 to 6c2c3ac Compare January 8, 2019 23:41
@dschinkel
Copy link

dschinkel commented Jan 12, 2019

related reduxjs/react-redux#1161

@minznerjosh
Copy link
Contributor Author

Btw I don’t think this implementation is quite right yet. It effectively auto-dives through as many Consumers/Providers as are necessary until it reaches a non Consumer/Provider. I think the correct behavior is for a Consumer/Provider to behave like any other component. Does that make sense?

I’ll take a look at implementing what I believe is the correct functionality in a couple of days.

@venikx

This comment has been minimized.

@dschinkel

This comment has been minimized.

@minznerjosh

This comment has been minimized.

@dschinkel

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Jan 19, 2019

I definitely don't want a "diveUntil" API as part of shallow; that's something I'm planning to tackle as an entirely separate top-level API after we're caught up with React's featureset.

I'm not even confident that enzyme should auto-dive Providers and Consumers yet, to be honest.

@venikx

This comment has been minimized.

@minznerjosh
Copy link
Contributor Author

@ljharb I don't think we should auto-dive Providers/Consumers. I think instead you should be able to manually dive() them like any other component. I'm going to fix-up this PR today so that calling dive() once does not end up rendering more than one level of Consumer/Provider.

@minznerjosh minznerjosh force-pushed the josh__dive-on-new-context branch from 6c2c3ac to f627d1e Compare January 19, 2019 13:15
@dschinkel

This comment has been minimized.

@ljharb

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Jan 19, 2019

@minznerjosh it makes sense that diving a Consumer/Provider should work as if it was a regular custom component with a function child - altho it's a bit surprising that it doesn't already do that without any enzyme changes.

@venikx

This comment has been minimized.

@ljharb

This comment has been minimized.

@dschinkel

This comment has been minimized.

@venikx

This comment has been minimized.

@dschinkel
Copy link

dschinkel commented Jan 22, 2019

@ljharb just wondering what you meant by:

Solving that problem is something I'm interesting in doing after we're caught up to React's featureset.

but then you also said:

I definitely don't want a "diveUntil" API as part of shallow; that's something I'm planning to tackle as an entirely separate top-level API after we're caught up with React's featureset.

I'm not even confident that enzyme should auto-dive Providers and Consumers yet, to be honest.

seems contradictory?

…ateContext()` providers and consumers

 - add `isContextConsumer`
 - add `getProviderFromConsumer`
@minznerjosh minznerjosh force-pushed the josh__dive-on-new-context branch from f627d1e to 5566ebe Compare January 22, 2019 20:13
@minznerjosh minznerjosh changed the title Add ability to dive() Consumers and Providers Add ability to dive() and shallow-render-as-root Consumers and Providers Jan 22, 2019
@eugenet8k
Copy link

Guys, any progress on submitting this PR? It is a big blocker for migration to react-redux v6 😭

@hally9k
Copy link

hally9k commented Mar 6, 2019

We too are blocked on this one, any update?

@ljharb
Copy link
Member

ljharb commented Apr 2, 2019

@minznerjosh i've freshly rebased this. Can we make sure to add test cases for both shallow and mount to cover this functionality, including both consumers and providers as the root, or with .dive, .shallow, and .mount?

@minznerjosh
Copy link
Contributor Author

@ljharb yup I'll give this PR some love tomorrow!

@minznerjosh minznerjosh force-pushed the josh__dive-on-new-context branch from d35a5fa to 492cd0d Compare April 3, 2019 15:34
@minznerjosh
Copy link
Contributor Author

@ljharb Updated!

  • Added mount() coverage for rendering a <Provider />/<Consumer /> as root (there was already a spec for rendering them as not root)
  • The shallow() tests from the first iteration already had coverage for rendering both <Provider /> and <Consumer /> as root, and dive()ing them both
  • Added the required logic for wrappingComponent in shallow() to support createContext() (just like legacy context has to be pulled off the <RootFinder />, so does createContext() context)

Looking forward to your review!

@minznerjosh

This comment has been minimized.

@ljharb ljharb force-pushed the josh__dive-on-new-context branch 2 times, most recently from e80e978 to 7360912 Compare April 4, 2019 06:53
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.

k, let's give this a shot :-)

@ljharb ljharb merged commit 2420e2b into enzymejs:master Apr 4, 2019
@minznerjosh minznerjosh deleted the josh__dive-on-new-context branch April 4, 2019 20:47
ljharb added a commit that referenced this pull request Apr 6, 2019
 - [new] `Utils` add `findElement`, `getNodeFromRootFinder`, `wrapWithWrappingComponent`, `getWrappingComponentMountRenderer`; add `RootFinder` (#1966)
 - [new] add `getDerivedStateFromError` support (#2036)
 - [fix] `shallow`/`mount`: `renderProp`: avoid warning when render prop returns `null` (#2076)
 - [build] include source maps
 - [deps] update `eslint`
 - [dev deps] update `eslint`, `semver`, `rimraf`, `react-is`, `html-element-map`, `chai`, `eslint-plugin-mocha`
ljharb added a commit that referenced this pull request Apr 6, 2019
 - [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
ljharb added a commit that referenced this pull request Apr 6, 2019
 - [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
@ljharb ljharb mentioned this pull request Apr 6, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants