-
Notifications
You must be signed in to change notification settings - Fork 668
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
feat: Introduce enableAutoDestroy() helper function #1245
Conversation
@souldzin can I ask you to take a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pushed the following changes:
- simplified the tests a little
- added a guard against calling the helper twice
- made the wrapper instances scoped inside Wrapper instead of the module to allow resetting them (for example in unit tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping and iterating on this @winniehell! Back to you ⚽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, two small comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should document the resetAutoDestroyState
? I am not sure if it makes sense for ppl to need it though 😆
I think if we have the feature, it should be documented. Maybe a sentence or two explaining when/why it's useful? The code seems good, no problems here 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
Regarding documenting resetAutoDestroyState
, we should do so, but I wouldn't stress it too much. Maybe just mention it in the enableAutoDestroy
section?
Same, just mention it exists. Maybe add them to the API docs, because I find myself search there more, than in the guides. |
This introduces a new
enableAutoDestroy()
helper function which allows to automatically callwrapper.destroy()
by passing in a custom hook function (such asafterEach
).closes #1236