[Head API]: Fixed runtime crash of monkey-patched console.error #39020
Conversation
gatsbot
bot
added
the
status: triage needed
| return undefined | ||
| } | ||
| } catch (e) { | ||
| originalConsoleError(...args, e) |
There was a problem hiding this comment.
wouldn't logging in this catch block and later on line 77 not result in mostly duplicated log lines (with first line showing caught error here)?
I don't think logging error we are catching here is beneficial to users overall and maybe just getting behavior of not "filtering out" calls that error out would be enough? As in we only want to filter out one specific line that wouldn't be throwing, but everything else should be just passed through to original console.error regardless if the checks throw or not
There was a problem hiding this comment.
@pieh In my case, I'm trying to console.error(an actual error), but it crashes with error inside the mocked function and I have no idea what exactly crashed and what I was trying to see as result of console.error. In both dev and prod.
Better approach would be not monkey-patching console.error at all, but that is not my call
Is there a specific reason to do that for Head api? Can we just remove whole thing?
There was a problem hiding this comment.
@pieh Why does tests failing? I see errors in unrelated to my changes packages
I've encountered an error in monkey-patched console.error. Monkey-patching native functions is bad practice in general, but in this case, I got unexpected runtime crash while handling a failed case of try catch in my code.
I've added try/catch block inside the monkey-patched function, so it handles errors gracefully.
Please review.
This is first of the series of PR I'm making, based on my previous research and huge PR