-
Notifications
You must be signed in to change notification settings - Fork 233
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
fix: reduce console.error suppressions to only while acting #542
Conversation
Codecov Report
@@ Coverage Diff @@
## master #542 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 16 +2
Lines 210 234 +24
Branches 23 26 +3
=========================================
+ Hits 210 234 +24
Continue to review full report at Codecov.
|
I'm struggling to understand though why we'd want to impliment this "just working" feat as opposed to getting users to add one param? This seems like overall it's more effort to maintain? |
I agree that it's more to maintain and we should be wary of that. The main reason to consider over #541 is that by limiting the side effect (replacing In #541, setting the option to I'm starting to think an environment variable escape hatch is worth it if we go this way. I'm still very much on the fence about it though as that act wrapper scares me a bit. |
Maybe let's release a beta, get the folks on discord to try it out & see how they feel – RFC. We could also add RE: Act wrapper, my concern is they change it one impl in either |
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.
Comments above
Added RHTL_DISABLE_ERROR_FILTERING environment variable toggle to globally disable console filtering. Fixes #546
That’s funny, I was about to pick this up, so do you think this should be the route to go with? Whilst incl. an ENV escape hatch? |
Even though the code here is a bit more hectic than #541, I feel like this would be the easiest to unwind later of we want to because it doesn't introduce a new option (other than the env variable) |
Feels like everyone else is saying go for #541 though 😕 |
Wouldn’t the variable work the same as the option? Therefore this PR is 541 but a smarter(?) |
Similar but not quite the same. The env variable is much more global than the option which is constrained to a single test. In practice though you'd probably get every test in a file passing the option and it might as well be global at that point. |
Closing in favour of #549 |
What:
Another option vs #541
Why:
Making it just work for more users without having to set options.
How:
By only suppressing the error boundary output while
act
ing, the test code retains access to the unaltered (or altered in their own way) version ofconsole.error
.Technically, the test code will have access to our suppressed version of
console.error
within anact
callback, however they would have to be using it for something other than logging an error (e.g. asserting a mocked version was called) within that callback to have an issue. Calling it to log an error with it would work as expected.There is an option to return the unaltered
act
to them and only use the wrapped version internally aroundrender
calls, which would alleviate the above issue, however that might leave them susceptible to showing the output if theiract
update caused and error in the subsequent render passes.The tradeoff with this implementation is that we cannot pass an option to disable it like in #541 as the
act
function needs to be wrapped in the global scope before the the renderer receives the options. We could potentially use an environment variable to disable is globally like we do for other things if we really want an escape hatch on it.Checklist:
The code here is pretty crude, especially in the
act
wrapper. I'll clean it up if we want to go this way.