-
Notifications
You must be signed in to change notification settings - Fork 232
fix: only suppress console.error for non-pure imports #549
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
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #549 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 14 15 +1 Lines 210 218 +8 Branches 23 30 +7 ========================================= + Hits 210 218 +8
Continue to review full report at Codecov.
|
I wasn't quite right about the docs and tests not needing updating. The docs were very close, but I have added more detail about the side effects, and the tests were still accurate, but there were some new cases that needed a test so I have refactored them a bit too.
So would the way to disable the console patching to be to use the /pure export of each module, else we continue to do what we're currently doing?
Using the pure import will disable it for the test file or setting the env variable (e.g. with the setup file utility) can disable it more globally. The docs cover these options.
docs/api-reference.md
Outdated
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.
Just reading what this list of links is for, I don't this this one should be added here after all (it will appear in the side menu still though).
Honestly, I'm feeling best about this option at the moment, especially now the suppression is being handled in before/after test hooks.
especially now the suppression is being handled in before/after test hooks.
Now that I've just said that I wonder if the "on import" of the previous change makes it slightly better for people wanting to mock console.error so that their mock is mock likely to occur after our patching.
Or moving to to beforeAll and afterAll instead of ...Each could also make sense as they are more likely to be using ...Each (not may people is ...All in my experience, but that might just be me).
... Back to confusion.
...can actually be registered
...uppression function
I've had a quick look at this, it does look promising. Resolving the pure import was a good step to take & i think taking advantage of *Each functions in Jest was a good idea too. I'm especially happy we don't seem to be wrapping act anymore?
I'm especially happy we don't seem to be wrapping act anymore
Correct. No act wrapping.
I think we should move forward with this one. It's not worse than what we have now, but also has the escape hatches if people need them.
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, well done for exploring so many options, I don't think I would have been able to tackle this issue so thoroughly 😅
Ok... deep breath... here we go...
Uh oh!
There was an error while loading. Please reload this page.
What:
Yet another alternative to #541 and #542
Why:
#546
How:
The complete opposite approach to #542 but the documentation and tests are still valid. By extending the suppression time from the initial import to after all tests have run, the patched
console.errorcan be used by other utilities when setting up local mocks, while the code remains relatively straight forward and we don't need to wrapact.Care here is taken to ensure that patching only occurs if the defaul imports are used and not the
/pureimports.Checklist:
Fixes #546