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

[Feature Request] Mount tracking #1689

Open
TzviPM opened this issue Jun 22, 2018 · 8 comments · May be fixed by #1730
Open

[Feature Request] Mount tracking #1689

TzviPM opened this issue Jun 22, 2018 · 8 comments · May be fixed by #1730
Labels
feature request Issues asking for stuff that would be semver-minor

Comments

@TzviPM
Copy link

TzviPM commented Jun 22, 2018

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:

  • if someone calls unmount manually, we probably want to delete any global references tot he underlying wrapper, so that the JS engine can properly garbage-collect it. It also doesn't make sense to call unmount multiple times anyhow.

  • Many applications will use something like mocked timers int heir testing suites. We'll need to consider the case where these utilities are referenced via a component's componentWillUnmount hook. In such a case, if the mock is removed after the test and then we try to run a global unmountAll hook on that component, we may encounter errors from the mocking utility. One way to handle this is via documentation, suggesting the package consumer to manually call unmount on those wrappers before restoring any mocked utilities.

@TzviPM TzviPM changed the title Mount tracking [Feature Request] Mount tracking Jun 22, 2018
@TzviPM
Copy link
Author

TzviPM commented Jun 22, 2018

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

@ljharb ljharb added the feature request Issues asking for stuff that would be semver-minor label Jul 4, 2018
@ljharb
Copy link
Member

ljharb commented Jul 4, 2018

@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
Copy link
Member

ljharb commented Jul 4, 2018

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

@TzviPM
Copy link
Author

TzviPM commented Jul 31, 2018

@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
Copy link
Member

ljharb commented Aug 2, 2018

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

@TzviPM
Copy link
Author

TzviPM commented Aug 2, 2018

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.

@TzviPM TzviPM linked a pull request Aug 2, 2018 that will close this issue
@alicederyn
Copy link

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
Copy link
Member

ljharb commented Apr 4, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues asking for stuff that would be semver-minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants