-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
implemented test and code changes to fix issue #20117, to enable hand… #20191
implemented test and code changes to fix issue #20117, to enable hand… #20191
Conversation
…ble handling of dev tools global hook being disabled
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f6a2054:
|
@@ -447,6 +447,7 @@ export function injectIntoGlobalHook(globalObject: any): void { | |||
globalObject.__REACT_DEVTOOLS_GLOBAL_HOOK__ = hook = { | |||
renderers: new Map(), | |||
supportsFiber: true, | |||
isDisabled: false, |
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.
nit: We don't need to add this value explicitly. if (hook.isDisabled)
will fail if it's undefined
(as it will be if DevTools is the thing that injects the hook first).
if (hook.isDisabled) { | ||
// This isn't a real property on the hook, but it can be set to opt out | ||
// of DevTools integration and associated warnings and logs. | ||
// https://github.com/facebook/react/issues/20117 |
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.
nit: Let's link to #11448
console.error( | ||
'The installed version of React DevTools is disabled. ' + | ||
'https://reactjs.org/link/react-devtools', | ||
); |
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.
This looks like the right type of check, but the error message isn't the clearest. It's not that React DevTools has been disabled. (It may not even have been installed.)
Maybe we could say something more like:
Something has shimmed the React DevTools global hook (REACT_DEVTOOLS_GLOBAL_HOOK). Fast Refresh is not compatible with this shim and will be disabled.
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.
When I use console.error(message)
the build process failed with the following message. This module has no access to the React object, so it can't use console.error() with the automatically appended stack.
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.
@bvaughn but if I use throw new Error('message')
the build will be successful.
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.
The rest of the warning message you're referring to should say to use console['error'](...)
instead of console.error(...)
.
react/scripts/babel/transform-replace-console-calls.js
Lines 50 to 63 in 13a62fe
if (path.node.callee.property.type !== 'Identifier') { | |
// Don't process calls like console['error'](...) | |
// because they serve as an escape hatch. | |
return; | |
} | |
if (path.get('callee').matchesPattern('console.error')) { | |
if (this.opts.shouldError) { | |
throw path.buildCodeFrameError( | |
"This module has no access to the React object, so it can't " + | |
'use console.error() with automatically appended stack. ' + | |
"As a workaround, you can use console['error'] which won't " + | |
'be transformed.' | |
); | |
} |
But in this case, I think we want to use warn
instead of error
anyway. (This isn't an error.) So use console["warn"](...)
.
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.
@bvaughn I'll submit a pull request and I'll be honored if it gets merged.
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.
I didn't submit a pull request earlier because the build was failing.
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.
You can check the issue ID to confirm I was the one who picked up the issue.
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.
Go ahead alphabet-codes and take it. Sorry, I didn't notice you had picked it. I'm pretty busy right now. I can be a fallback if needed to resolve this issue.
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.
@bbeckercscc thanks, I'm really honored
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.
…ling of dev tools global hook being disabled
Summary
Test Plan