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 |
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
acting, 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.errorwithin anactcallback, 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
actto them and only use the wrapped version internally aroundrendercalls, which would alleviate the above issue, however that might leave them susceptible to showing the output if theiractupdate 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
actfunction 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
actwrapper. I'll clean it up if we want to go this way.