Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Head API]: Fixed runtime crash of monkey-patched console.error #39020

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[Head API]: Fixed runtime crash of monkey-patched console.error #39020

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c62e544
fixed runtime crash of monkeypatched console.error
JustFly1984 Jun 23, 2024
3033226
fixed prettier error
JustFly1984 Jun 23, 2024
a0d775e
Merge branch 'master' into fix/console.error-monkeypatching
JustFly1984 Jun 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions packages/gatsby/cache-dir/head/head-export-handler-for-browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,19 @@ if (process.env.BUILD_STAGE === `develop`) {
// https://github.com/facebook/react/blob/e2424f33b3ad727321fc12e75c5e94838e84c2b5/packages/react-dom-bindings/src/client/validateDOMNesting.js#L498-L520
const originalConsoleError = console.error.bind(console)
console.error = (...args) => {
if (
Array.isArray(args) &&
args.length >= 2 &&
args[0]?.includes?.(`validateDOMNesting(...): %s cannot appear as`) &&
(args[1] === `<html>` || args[1] === `<body>`)
) {
return undefined
try {
if (
Array.isArray(args) &&
args.length >= 2 &&
args[0]?.includes?.(`validateDOMNesting(...): %s cannot appear as`) &&
(args[1] === `<html>` || args[1] === `<body>`)
) {
return undefined
}
} catch (e) {
originalConsoleError(...args, e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

}

return originalConsoleError(...args)
}

Expand Down