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

implemented test and code changes to fix issue #20117, to enable hand… #20191

Closed
wants to merge 1 commit into from
Closed

implemented test and code changes to fix issue #20117, to enable hand… #20191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
197 changes: 104 additions & 93 deletions packages/react-refresh/src/ReactFreshRuntime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

inject(injected) {
return nextID++;
},
Expand All @@ -465,107 +466,117 @@ export function injectIntoGlobalHook(globalObject: any): void {
};
}

// Here, we just want to get a reference to scheduleRefresh.
const oldInject = hook.inject;
hook.inject = function(injected) {
const id = oldInject.apply(this, arguments);
if (
typeof injected.scheduleRefresh === 'function' &&
typeof injected.setRefreshHandler === 'function'
) {
// This version supports React Refresh.
helpersByRendererID.set(id, ((injected: any): RendererHelpers));
}
return id;
};
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 •
edited
Loading

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

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 •
edited
Loading

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

} else {
// Here, we just want to get a reference to scheduleRefresh.
const oldInject = hook.inject;
hook.inject = function(injected) {
const id = oldInject.apply(this, arguments);
if (
typeof injected.scheduleRefresh === 'function' &&
typeof injected.setRefreshHandler === 'function'
) {
// This version supports React Refresh.
helpersByRendererID.set(id, ((injected: any): RendererHelpers));
}
return id;
};

// Do the same for any already injected roots.
// This is useful if ReactDOM has already been initialized.
// https://github.com/facebook/react/issues/17626
hook.renderers.forEach((injected, id) => {
if (
typeof injected.scheduleRefresh === 'function' &&
typeof injected.setRefreshHandler === 'function'
) {
// This version supports React Refresh.
helpersByRendererID.set(id, ((injected: any): RendererHelpers));
}
});
// Do the same for any already injected roots.
// This is useful if ReactDOM has already been initialized.
// https://github.com/facebook/react/issues/17626
hook.renderers.forEach((injected, id) => {
if (
typeof injected.scheduleRefresh === 'function' &&
typeof injected.setRefreshHandler === 'function'
) {
// This version supports React Refresh.
helpersByRendererID.set(id, ((injected: any): RendererHelpers));
}
});

// We also want to track currently mounted roots.
const oldOnCommitFiberRoot = hook.onCommitFiberRoot;
const oldOnScheduleFiberRoot = hook.onScheduleFiberRoot || (() => {});
hook.onScheduleFiberRoot = function(
id: number,
root: FiberRoot,
children: ReactNodeList,
) {
if (!isPerformingRefresh) {
// If it was intentionally scheduled, don't attempt to restore.
// This includes intentionally scheduled unmounts.
failedRoots.delete(root);
if (rootElements !== null) {
rootElements.set(root, children);
// We also want to track currently mounted roots.
const oldOnCommitFiberRoot = hook.onCommitFiberRoot;
const oldOnScheduleFiberRoot = hook.onScheduleFiberRoot || (() => {});
hook.onScheduleFiberRoot = function(
id: number,
root: FiberRoot,
children: ReactNodeList,
) {
if (!isPerformingRefresh) {
// If it was intentionally scheduled, don't attempt to restore.
// This includes intentionally scheduled unmounts.
failedRoots.delete(root);
if (rootElements !== null) {
rootElements.set(root, children);
}
}
}
return oldOnScheduleFiberRoot.apply(this, arguments);
};
hook.onCommitFiberRoot = function(
id: number,
root: FiberRoot,
maybePriorityLevel: mixed,
didError: boolean,
) {
const helpers = helpersByRendererID.get(id);
if (helpers !== undefined) {
helpersByRoot.set(root, helpers);

const current = root.current;
const alternate = current.alternate;

// We need to determine whether this root has just (un)mounted.
// This logic is copy-pasted from similar logic in the DevTools backend.
// If this breaks with some refactoring, you'll want to update DevTools too.

if (alternate !== null) {
const wasMounted =
alternate.memoizedState != null &&
alternate.memoizedState.element != null;
const isMounted =
current.memoizedState != null &&
current.memoizedState.element != null;

if (!wasMounted && isMounted) {
return oldOnScheduleFiberRoot.apply(this, arguments);
};
hook.onCommitFiberRoot = function(
id: number,
root: FiberRoot,
maybePriorityLevel: mixed,
didError: boolean,
) {
const helpers = helpersByRendererID.get(id);
if (helpers !== undefined) {
helpersByRoot.set(root, helpers);

const current = root.current;
const alternate = current.alternate;

// We need to determine whether this root has just (un)mounted.
// This logic is copy-pasted from similar logic in the DevTools backend.
// If this breaks with some refactoring, you'll want to update DevTools too.

if (alternate !== null) {
const wasMounted =
alternate.memoizedState != null &&
alternate.memoizedState.element != null;
const isMounted =
current.memoizedState != null &&
current.memoizedState.element != null;

if (!wasMounted && isMounted) {
// Mount a new root.
mountedRoots.add(root);
failedRoots.delete(root);
} else if (wasMounted && isMounted) {
// Update an existing root.
// This doesn't affect our mounted root Set.
} else if (wasMounted && !isMounted) {
// Unmount an existing root.
mountedRoots.delete(root);
if (didError) {
// We'll remount it on future edits.
failedRoots.add(root);
} else {
helpersByRoot.delete(root);
}
} else if (!wasMounted && !isMounted) {
if (didError) {
// We'll remount it on future edits.
failedRoots.add(root);
}
}
} else {
// Mount a new root.
mountedRoots.add(root);
failedRoots.delete(root);
} else if (wasMounted && isMounted) {
// Update an existing root.
// This doesn't affect our mounted root Set.
} else if (wasMounted && !isMounted) {
// Unmount an existing root.
mountedRoots.delete(root);
if (didError) {
// We'll remount it on future edits.
failedRoots.add(root);
} else {
helpersByRoot.delete(root);
}
} else if (!wasMounted && !isMounted) {
if (didError) {
// We'll remount it on future edits.
failedRoots.add(root);
}
}
} else {
// Mount a new root.
mountedRoots.add(root);
}
}

// Always call the decorated DevTools hook.
return oldOnCommitFiberRoot.apply(this, arguments);
};
// Always call the decorated DevTools hook.
return oldOnCommitFiberRoot.apply(this, arguments);
};
}
} else {
throw new Error(
'Unexpected call to React Refresh in a production environment.',
Expand Down
15 changes: 15 additions & 0 deletions packages/react-refresh/src/__tests__/ReactFresh-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,21 @@ describe('ReactFresh', () => {
}
});

it('throws an error when react dev tools global hook is disabled to avoid crashing', () => {
if (__DEV__) {
global.__REACT_DEVTOOLS_GLOBAL_HOOK__.isDisabled = true;
ReactFreshRuntime = require('react-refresh/runtime');

expect(() => {
ReactFreshRuntime.injectIntoGlobalHook(global);
}).toErrorDev(
'The installed version of React DevTools is disabled. ' +
'https://reactjs.org/link/react-devtools',
{withoutStack: true},
);
}
});

it('can update forwardRef render function in isolation', () => {
if (__DEV__) {
render(() => {
Expand Down