enzymejs / enzyme

JavaScript Testing utilities for React
https://enzymejs.github.io/enzyme/
MIT License
19.96k stars 2.01k forks source link

[Feature Request] Mount tracking #1689

Open TzviPM opened 6 years ago

TzviPM commented 6 years ago

Is your feature request related to a problem? Please describe.

When using full DOM rendering, I'd like to be able to orchestrate calling unmount via an afterEach-style hook, rather than having to call unmount in each test.

Note When unmount was not called at all, I experienced memory leaks when working with large test suites.

Describe the solution you'd like

Obviously, enzyme does not assume a specific test environment. I think the right solution here is for enzyme to provide some form of mount-tracking with an unmountAll function.

Describe alternatives you've considered

Currently, we expose a library which wraps enzyme's mount function, tracking the ReactWrapper instances, and then calling unmount on them via our library.

This solution is problematic, as most developers are familiar with import {mount} from 'ezyme';, and we'd end up needing a lint rule to prevent that. Also, I feel that this solution makes sense to be included in enzyme itself so that others can benefit from better handling of memory leaks.

Additional context

Some implementation considerations:

TzviPM commented 6 years ago

@ljharb would you be open to a PR for this functionality?

ljharb commented 6 years ago

@tzvipm before a PR, this needs extensive discussion. How would you implement this in a way that's adapter-agnostic, and that wouldn't have a ton of memory leaks?

ljharb commented 6 years ago

(also, I'd expect the same functionality to be optionally available for shallow)

TzviPM commented 6 years ago

@ljharb I apologize for the delay in replying to the above.

In order to accomplish what I'm suggesting, I believe we'd need to store a global reference to each mounted node. Thus, if the user never calls the global unmount hook, we'd end up saving those references indefinitely, worsening the existing problem.

Because of this, I believe that we should consider making the mount tracking feature an opt-in via a flag. If that flag is not set, then we never store references at all. When the flag is set, we can store references to each wrapper on mount (or even shallow), then provide a global function for unmountAllWrappers at which point we call unmount on each wrapper, and then remove the reference to it. This way, the wrappers can be properly garbage-collected.

All this said, I can't help but wonder why we wouldn't make this an always-on feature, thereby providing a more robust experience tot he masses by default. This may require a more major update to the API, requiring the user to call some sort of cleanup function.That said, we already require a setup step using adapters, so perhaps this is fine.

Next, in terms of adapters, I think we can simply delegate the functionality to the unmount method of the wrapper, so any adapter agnosticism would be inherited from that which is already baked in to the library.

ljharb commented 6 years ago

Making it opt-in seems like it's worth exploring a PR for.

TzviPM commented 6 years ago

I'll put up a PR shortly. Thanks for your help @ljharb. I may need a bit of help on the docs side of things as well, as this is my first PR for enzyme.

alicederyn commented 5 years ago

How about letting the user add a callback that will be called every time mount() is called? Then they can add their own automatic cleanup, with no global flags and no risk of memory leaks?

ljharb commented 5 years ago

@alicederyn that's not a bad idea, but that seems like it would just be pushing the work/responsibility of ensuring no memory leaks to every user of that feature - instead of solving it once in enzyme directly. #1730 seems like a worthy direction to explore to me, still.