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

Projects
7 participants
@minznerjosh
Copy link
Contributor

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(),
} = {}) {

This comment has been minimized.

Copy link
@minznerjosh

minznerjosh Jan 5, 2019

Author Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

minznerjosh commented Jan 7, 2019

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 trialspark:josh__dive-on-new-context branch from 041cdc0 to d2e3db0 Jan 8, 2019

@minznerjosh

This comment has been minimized.

Copy link
Contributor Author

minznerjosh commented Jan 8, 2019

@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 trialspark:josh__dive-on-new-context branch from d2e3db0 to 6c2c3ac Jan 8, 2019

@dschinkel

This comment has been minimized.

Copy link

dschinkel commented Jan 12, 2019

@minznerjosh

This comment has been minimized.

Copy link
Contributor Author

minznerjosh commented Jan 14, 2019

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.

@Fandekasp Fandekasp referenced this pull request Jan 15, 2019

Closed

Dive shallow in a Context Consumer #1908

2 of 2 tasks complete
@AnaRobynn

This comment was marked as off-topic.

Copy link

AnaRobynn commented Jan 18, 2019

Why limiting only diving Providers/Consumers. Same problems with shallow occur when using a HOC.

Have you thought about a functionality like: diveUntil?

I’m super oblivious to the technical implications, probably. But from an outside perspective seems very similar behavior.

HOC => diveUntil component you want to assert
Provider => diveUntil component you want to assert

@dschinkel

This comment was marked as off-topic.

Copy link

dschinkel commented Jan 18, 2019

@AnaRobynn agree.

We should still be able to still:
shallow + store prop / (still be able to dive when necessary on an HOC or Host) - for isolated tests when not using or using context API (we should have flexibility there, not just forced to use the context API)
mount() + provider + store prop - for integration tests

and in either situation specify a store as prop. This ability should never change on enzyme, React, or Redux side because people rely on writing tests this way. I should not have to worry about tests I've written over the past 3 years to break nor should I have to change my style of writing tests especially in the case of TDD where we rely on shallow and simplicity.

@minznerjosh

This comment was marked as off-topic.

Copy link
Contributor Author

minznerjosh commented Jan 18, 2019

@AnaRobynn a feature like that is definitely possible to implement. But that’s out of scope for this PR. Presumably diveUntil() (that supports consumers and providers) would be built on top of the work done in this PR.

You might also be interested in this library, which adds this functionality to enzyme today! https://www.npmjs.com/package/enzyme-shallow-until

@dschinkel

This comment was marked as off-topic.

Copy link

dschinkel commented Jan 18, 2019

Yea though one issue with a recursive dive is that you are basically putting a bandaid on really what you shouldn't...meaning losing some design feedback though. Because sure, it might make your tests less fragile if you add say another HOC or renderProps or something else, but you actually want dive() to be painful because that helps give you design pressure that says "Me the component is painful to test, because you need to make my simpler and break me apart". What happens is people resort to that or mount() to "get rid of the pain" and then they miss the point, and therefore never see other issues with their design and maybe why it "hurts" and so they start only creating integration style tests which does not give you the bang for your buck.

I could very easily just add a helper that recursively dives but have hesitated due to the fact above. I want that design feedback when I TDD.

I hope enzyme never actually adds such a method to dive honestly. Because then that would lure devs into a trap (not intentionally) of trying to "make my tests never break" which is too black and white and the wrong goal. Your tests will break and even though you're adding an implementation detail such as wrapping your component inside another HOC, you should see that you're ending up with way too many nested things in your component. Recursive dive will just allow people to ignore that feedback...and so for enzyme, if they want to ignore that and write all integration tests (which I think is a bad route) then they would naturally resort to mount.

@ljharb

This comment has been minimized.

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.

@AnaRobynn

This comment was marked as off-topic.

Copy link

AnaRobynn commented Jan 19, 2019

@ljharb @dschinkel Sounds reasonable.

Let’s say I have a component, I want to inject styles via a HOC (similar to material-ui).
I’m wondering if my tests should break, just because I’m adding this styling HOC or not.

I not clear to me what the benefits/drawbacks are for for either failing/not failing a unit test, due to adding a HOC.

But that’s irrelevant in this PR.

@minznerjosh

This comment has been minimized.

Copy link
Contributor Author

minznerjosh commented Jan 19, 2019

@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 trialspark:josh__dive-on-new-context branch from 6c2c3ac to f627d1e Jan 19, 2019

@dschinkel

This comment was marked as off-topic.

Copy link

dschinkel commented Jan 19, 2019

I’m wondering if my tests should break, just because I’m adding this styling HOC or not

Good question I'll try to tackle that question in my upcoming TDD course...which will now be FREE!!!!! YAY! (especially since I use shallow 100% of the time)

@ljharb

This comment was marked as off-topic.

Copy link
Member

ljharb commented Jan 19, 2019

@AnaRobynn yes, adding an HOC is a breaking change for a shallow test - you have to add one dive per HOC. Solving that problem is something I'm interesting in doing after we're caught up to React's featureset.

@ljharb

This comment has been minimized.

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.

@AnaRobynn

This comment was marked as off-topic.

Copy link

AnaRobynn commented Jan 19, 2019

@ljharb I know it’s a breaking change. I’m wondering if it should break a shallow test or not.

@ljharb

This comment was marked as off-topic.

Copy link
Member

ljharb commented Jan 19, 2019

@AnaRobynn no, adding a well-behaved HOC should not be a breaking change for any consumers except shallow, for which it should require one additional .dive() call.

@dschinkel

This comment was marked as off-topic.

Copy link

dschinkel commented Jan 19, 2019

This is a statement to nobody specific. But, we need to be careful saying that "everything is a problem". The JS community especially tends to do this because there is not a lot of experienced devs on writing tests. So typically they look for something that gives them an out rather than realizing that you should not look for an out every single time..it has a lot to do with your design...and we should look at our design first.

Shallow, think about it. It's very specific. It's sensitive because of its nature. That's a god thing when you're focused on lower-level isolated tests. You have to expect that if your design is not clean and simple, you're going to have breaking changes if you introduce HOCs or more breaking changes if your design is doing too much or decoupled. Again that's design feedback, and that's a good thing and one should listen to it. Most devs don't understand that. I know you do @AnaRobynn but just making a general statement here. This is a big reason why the context of TDD helps and why those who do TDD already see this (because TDD forces you to see this early and very frequently as you go) and those who write tests after don't. I'm not putting anyone down, it's just fact. So I'm trying to make people aware of this, here and also in my upcoming vids.

For example when I TDD an app, sometimes refactoring does introduce breaking changes because you're not refactoring, you're actually changing the design and you may not realize it (you're changing the contract and you don't realize it for example)..you might see things too black and white when you're in the mode to make things cleaner. It's a subtle difference sometimes that people mistakenly say "my tests should never break" which is just not true. You have to practice and you'll come to realize those subtle differences between "Am I really refactoring?" or am I really "changing my design during when I think I'm refactoring".

@AnaRobynn

This comment was marked as off-topic.

Copy link

AnaRobynn commented Jan 19, 2019

@dschinkel @ljharb
True, if you really think about shallow component is that props and what gets rendered are the api. So in theory wrapping some component in a HOC is breaking that API.

Anyways, I do not further want to derail this PR with unrelated questions. Thank you for replying/explaining my concerns/questions. Really helpful.

@dschinkel

This comment has been minimized.

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?

minznerjosh added some commits Jan 22, 2019

[enzyme-adapter-react-{16,16.3}] [new] support shallow rendering `cre…
…ateContext()` providers and consumers

 - add `isContextConsumer`
 - add `getProviderFromConsumer`

@minznerjosh minznerjosh force-pushed the trialspark:josh__dive-on-new-context branch from f627d1e to 5566ebe Jan 22, 2019

@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

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 23, 2019

Sorry for the typo.

Yes, I'm saying that a Provider and Consumer should be no different from a user-created React component - and treated as if they were a user-created one.

I'm not talking about this PR specifically yet - just talking in generalities about how I expect things to work. It's highly likely that this PR already does exactly what I hope it to :-D i just haven't yet had the time to properly review it.

@minznerjosh

This comment has been minimized.

Copy link
Contributor Author

minznerjosh commented Jan 23, 2019

No worries! 😊 I was interpreting your earlier comments as “I’m not going to review this yet because there are major architectural/thematic flaws that need to be fixed before we get into the nitty-gritty of a code review.”

I’m hopeful this implementation does what you expect too! So now I’m going to chill out and wait for you to review. 😎

@eugenet8k

This comment has been minimized.

Copy link

eugenet8k commented Feb 7, 2019

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

@hally9k

This comment has been minimized.

Copy link

hally9k commented Mar 6, 2019

We too are blocked on this one, any update?

@ljharb ljharb force-pushed the trialspark:josh__dive-on-new-context branch from 7b08b1d to d35a5fa Apr 2, 2019

@ljharb

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

minznerjosh commented Apr 2, 2019

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

@minznerjosh minznerjosh force-pushed the trialspark:josh__dive-on-new-context branch from d35a5fa to 492cd0d Apr 3, 2019

@minznerjosh

This comment has been minimized.

Copy link
Contributor Author

minznerjosh commented Apr 3, 2019

@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 was marked as resolved.

Copy link
Contributor Author

minznerjosh commented Apr 3, 2019

@ljharb P.S. the cause of the build failure appears to be flakiness—not failing tests.

@ljharb ljharb force-pushed the trialspark:josh__dive-on-new-context branch 2 times, most recently from e80e978 to 7360912 Apr 4, 2019

@ljharb

ljharb approved these changes Apr 4, 2019

Copy link
Member

ljharb left a comment

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

@ljharb ljharb merged commit 2420e2b into airbnb:master Apr 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 91.626%
Details

@minznerjosh minznerjosh deleted the trialspark:josh__dive-on-new-context branch Apr 4, 2019

ljharb added a commit that referenced this pull request Apr 6, 2019

[enzyme-adapter-utils] v1.11.0
 - [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

[enzyme-adapter-react-16.3] v1.7.0
 - [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

[enzyme-adapter-react-16] v1.12.0
 - [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 referenced this pull request Apr 6, 2019

Open

[WIP] Support React hooks `useState` #2008

4 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.