[Shallow] Implement callEffects option to call effects from shallow renderer (#15275)

[insidewhy]

Fixes the effects related part of #15275 by allowing the user to tell the shallow renderer to call effect hooks.

It could do with a couple more tests but I wanted to get feedback on the approach and was also worried about putting any more time into it given that the maintainers have not responded to the corresponding issue in the months it's been open.

Added a full set of tests.

[sizebot]

Details of bundled changes.

Comparing: 606f76b...9e6ad0a

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactTestRenderer-dev.js	0.0%	-0.0%	602.38 KB	602.38 KB	125.73 KB	125.72 KB	FB_WWW_DEV
react-test-renderer-shallow.development.js	+5.4%	+3.7%	39.51 KB	41.64 KB	9.95 KB	10.32 KB	UMD_DEV
react-test-renderer-shallow.production.min.js	🔺+8.2%	🔺+6.2%	11.66 KB	12.62 KB	3.56 KB	3.78 KB	UMD_PROD
react-test-renderer-shallow.development.js	+6.3%	+4.3%	33.64 KB	35.78 KB	8.54 KB	8.91 KB	NODE_DEV
react-test-renderer-shallow.production.min.js	🔺+8.1%	🔺+6.3%	11.81 KB	12.76 KB	3.69 KB	3.92 KB	NODE_PROD
react-test-renderer.development.js	0.0%	-0.0%	590.1 KB	590.1 KB	126.25 KB	126.24 KB	UMD_DEV
react-test-renderer.production.min.js	0.0%	-0.0%	69.05 KB	69.05 KB	21.15 KB	21.15 KB	UMD_PROD
ReactShallowRenderer-dev.js	+6.7%	+4.7%	33.63 KB	35.87 KB	8.38 KB	8.78 KB	FB_WWW_DEV
react-test-renderer.development.js	0.0%	-0.0%	585.64 KB	585.64 KB	125.15 KB	125.15 KB	NODE_DEV
react-test-renderer.production.min.js	0.0%	-0.0%	68.78 KB	68.78 KB	20.93 KB	20.93 KB	NODE_PROD

Generated by 🚫 dangerJS

[bdwain]

thanks for figuring this out! If you want to copy the logic for componentDidMount and componentDidUpdate, you can see it at #15589

[insidewhy]

@bdwain I thought it would be good to handle the life-cycle events in a different PR. Since all my code is using hooks now, I'm not so interested in them. Plus it's easy to call them from outside if you need them, I see that enzyme already does that.

[euroclydon37]

Lol. I need this in my testing life.

[insidewhy]

Closed by accident, sorry.

[ssunday]

@ohjames Thank you so much for doing this! I'm not as familiar with the react code base (clearly from my comments) but if there's anything you need help on to get this going let me know 😬So eager to have this functionality in place.

[insidewhy]

@ssunday Nothing more we can really do for this until the maintainers have a look. They haven't commented on any of the related issues and they've been open for months. It's possible to get this working in your own extension of this library by hooking into one private method on ReactShallowRenderer, that's what I'm doing for now. Enzyme could possibly do the same.

[ssunday]

@ohjames Yeah, it'll take a while. With a draftjs change I was following, took a decent amount of time. But it happened!

For the hooking into private methods, could you link a gist or some examples of how to do that you could add to your PR comment? I think that helped with this draftjs PR— provide a way for people to rest in the wild and confirm? Or post to the Enzyme issue related to this. If it's not too difficult?

[nommuna]

This addition would help immensely when testing hooks with enzyme. Hopefully this gets added soon.

@ohjames what do you mean by “hooking into private methods”? Right now I am trying to find a good way to test private functions under a functional component and I’m not having any luck with it.

[ljharb]

Functional components can't have "methods"; that's a term that applies only to classes. If you mean, functions created inside the function, or only accessible via closure, you can't and shouldn't directly test those.

[bdwain]

yea i think @ohjames was referring to private methods on the shallow renderer itself

[insidewhy]

@ljharb I mean you can make the react test renderer support effects if you override one of its "private" (underscore prefixed) methods and then flush the hooks after each call to render.

[gaearon]

What API do you expect to need? Shallow renderer doesn't depend on any other code so I don't expect it to need private APIs. The only one I can think of is the way Hooks Dispatcher is exposed from the react package — but that isn't Enzyme-specific.

[gaearon]

We'd be happy to run some tests against a separate package fwiw. Similar to how we have integration tests with create-react-class.

[insidewhy]

The shallow renderer uses the following modules from "shared" that I don't think are externally reachable:

import describeComponentFrame from 'shared/describeComponentFrame';
import getComponentName from 'shared/getComponentName';
import shallowEqual from 'shared/shallowEqual';
import invariant from 'shared/invariant';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import warning from 'shared/warning';
import is from 'shared/objectIs';
import type { ReactContext, ReactEventResponderListener } from 'shared/ReactTypes';
import type {ReactElement} from 'shared/ReactElementType';

[ljharb]

Note that I've previously manually extracted shallowEqual as https://www.npmjs.com/package/enzyme-shallow-equal, and getComponentName as https://unpkg.com/browse/[email protected]/build/helpers/getComponentName.js - but it'd be much better if I could deprecate both of those to use official React packages, or alternatively, if React could use those instead of its current implementation.

Also, is exists as https://npmjs.com/object-is (same thing tho, it'd be ideal if React just used that package).

[roy-law]

Sorry I haven't replied earlier!

Generally we've been using shallow renderer less and less at Facebook because it encourages brittle tests that verify implementation detail. So components that relied on it a lot introduced a lot of refactoring friction, and didn't catch valuable regressions anyway for us. (Your experience might differ.)

The main value proposition of shallow rendering is that you mock out the inner components. We see the value in this, but in our experience doing this at the module system level gives you more leeway when refactoring. For example, you can always jest.mock('Button', () => 'button') to replace a child Button component with a mock. We've mostly been happy with this approach and found it more flexible.

Since we don't use the shallow renderer much anymore, we don't feel like we would be good stewards of its API, or even can recommend it. Additionally, Enzyme has been adding different behavior to it (such as calling class commit phase lifecycles) that we didn't originally plan. At least some of it was due to a misunderstanding (for example, React intends to guarantee refs are resolved in the commit phase, but running code with shallow renderer forces you to handle null gracefully). But there is also some mismatch between how Enzyme uses it internally, and how we initially thought it would be used.

At this point I think it would be better if we could move out the shallow renderer out of this repository. It's stagnating here, getting fixes very late, and causes frustration like in this PR. By design, it doesn't use almost any of the React code, so it should be easy to move. On the other hand, Enzyme is in active development and might as well want to take ownership of this piece.

If @davidmarkclements were kind to offer the react-shallow-renderer package name, we could externalize it there, and maybe it could be maintained in the Enzyme repository or elsewhere instead.

Would anyone like to propose a plan? Thanks.

What do you recommend using instead?

[ljharb]

I believe part of the mismatch here is that Facebook largely/historically doesn't do serverside rendering, which is where enzyme (and the shallow renderer) is critical. I'm not aware of any scalable alternatives.

[insidewhy]

@ljharb @gaearon How about just renaming _createDispatcher to createDispatcher. By making it part of the public interface of the class, we can hook into it and implement whatever effects we want from outside?

In the long term, to extract the shallow renderer, couldn't the shared package just be published?

But please consider implementing my first proposal. In the react-native world the shallow renderer is very popular, not being able to use (test) hooks makes us very sad.

[davidmfoley]

This would be quite useful to me and to other experienced TDD practitioners who work with react.

In addition to being on the order of 100x faster to run as compared to tests that require a stubbed DOM (i.e. JSDOM), shallow rendering allows for test-driving the designs of components much more efficiently/directly than the equivalent virtual DOM tests. Certain classes of components (for example: Providers that encapsulate state machines and provide to their children via context) are much, much easier to test-drive with shallow approaches.

Please consider merging this or otherwise exposing hooks to easily integrate effects into shallow test tools like enzyme in a way that is reliable and relatively future-proof.

[kumar303]

Generally we've been using shallow renderer less and less at Facebook because it encourages brittle tests

@gaearon just curious, is that because those specific components don't have static typing (like Flow) to enforce that nested components get instantiated correctly? Even with Flow, it's sometimes tricky to truly enforce component signatures, e.g. when using poorly typed HOCs.

It's true that shallow rendering won't expose problems like passing invalid props but static typing can solve this when done correctly.

The main value proposition of shallow rendering is that you mock out the inner components. We see the value in this, but in our experience doing this at the module system level gives you more leeway when refactoring. For example, you can always jest.mock('Button', () => 'button') to replace a child Button component with a mock. We've mostly been happy with this approach and found it more flexible.

Huh. To me, it seems way more flexible to rely on shallow rendering for this because the test doesn't have to know which dependencies to mock. This is important for refactoring; a test should not break when you add/remove nested components since they are not relevant to the main component under test -- nested components already have their own responsibilities.

I also like how shallow rendering encourages the test author to only consider functionality that the component under test is responsible for.

Since we don't use the shallow renderer much anymore, we don't feel like we would be good stewards of its API, or even can recommend it.

That makes sense, I was just curious to understand why, in case I'm missing some flaws in shallow rendering.

[thedanchez]

Any updates on this idea of creating a react-shallow-renderer package (and merging this PR into it) that can be maintained in isolation or within Enzyme?

[davidmarkclements]

@gaearon btw I'm happy to donate the package name when you guys are ready.
I'd also like to talk to someone about contributing https://github.com/esxjs/esx to the project in some way - any pointers?

[rmlevangelio]

This will make unit tests with React.16 easier

[Fieel]

I'd also love to keep using enzyme to test my stateful functional components

[thedanchez]

Following up on this on a weekly basis to make sure things don't go stale. Any updates?

[pyyding]

I'm late to the party but would like to say testing is important hence this pull request is important. Please merge. Thx 😊

[Fieel]

@ohjames, to be honest, this response is borderline unacceptable and I don't even know what to reply. Wow.

Can someone reopen the issue?

[gaearon]

@ohjames I recognize you're frustrated with the lack of movement on this but doing personal attacks on people working on this repository is not acceptable.

As I said in my earlier comment:

At this point I think it would be better if we could move out the shallow renderer out of this repository.

There is literally nothing that would prevent you from doing so and collaborating with e.g. Enzyme on it. The shallow renderer can be copy pasted, published in its own package, and be developed there.

I'm sorry that you were waiting for us to merge this PR, but I thought we set the expectation clear right above by saying we don't intend to invest more into shallow renderer and expanding its API surface. I don't see what kind of action you were expecting on our side.

[gaearon]

I believe part of the mismatch here is that Facebook largely/historically doesn't do serverside rendering, which is where enzyme (and the shallow renderer) is critical.

This is not correct btw; we do rely heavily on server rendering for the new Facebook.

[gaearon]

I'm going to lock this PR because the discussion has turned in an unconstructive direction. Additionally, @ljharb said he can't comment on the PR for some reason (maybe a GitHub bug).

I opened a new issue to discuss this in #17321. I welcome everyone from this thread (but please let's keep this one civil).