-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Comments
@ljharb would you be open to a PR for this functionality? |
@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? |
(also, I'd expect the same functionality to be optionally available for |
@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 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 Next, in terms of adapters, I think we can simply delegate the functionality to the |
Making it opt-in seems like it's worth exploring a PR for. |
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 |
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? |
@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. |
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 callunmount
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 callingunmount
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 callunmount
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 globalunmountAll
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 callunmount
on those wrappers before restoring any mocked utilities.The text was updated successfully, but these errors were encountered: