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.

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

Conversation

bbeckercscc
Copy link

…ling of dev tools global hook being disabled

Summary

Test Plan

…ble handling of dev tools global hook being disabled
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 7, 2020

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:

Sandbox Source
React Configuration

@@ -447,6 +447,7 @@ export function injectIntoGlobalHook(globalObject: any): void {
globalObject.__REACT_DEVTOOLS_GLOBAL_HOOK__ = hook = {
renderers: new Map(),
supportsFiber: true,
isDisabled: false,
Copy link
Contributor

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
Copy link
Contributor

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',
);
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@bvaughn bvaughn Nov 9, 2020

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(...).

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"](...).

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Author

@bbeckercscc bbeckercscc Nov 9, 2020

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.

Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@bvaughn can you please review my pull request #20208

@bvaughn bvaughn self-assigned this Nov 9, 2020
@bvaughn bvaughn closed this Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants