Skip to content

Commit

Permalink
Fast Refresh: Don't block DevTools commit hook (facebook#20129)
Browse files Browse the repository at this point in the history 
In some scenarios (either timing dependent, or pre-FR compatible React versions) FR blocked calling the React DevTools commit hook. This PR adds a test and a fix for that.
  • Loading branch information
Brian Vaughn authored Oct 29, 2020
1 parent b6a750b commit 343d7a4
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 59 deletions.
6 changes: 5 additions & 1 deletion packages/react-refresh/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,9 @@
},
"engines": {
"node": ">=0.10.0"
},
"devDependencies": {
"react-16-8": "npm:[email protected]",
"react-dom-16-8": "npm:[email protected]"
}
}
}
84 changes: 42 additions & 42 deletions packages/react-refresh/src/ReactFreshRuntime.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -517,53 +517,53 @@ export function injectIntoGlobalHook(globalObject: any): void {
didError: boolean,
) {
const helpers = helpersByRendererID.get(id);
if (helpers === undefined) {
return;
}
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) {
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);
};
} else {
Expand Down
75 changes: 59 additions & 16 deletions packages/react-refresh/src/__tests__/ReactFresh-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3745,24 +3745,32 @@ describe('ReactFresh', () => {
}
});

// This simulates the scenario in https://github.com/facebook/react/issues/17626.
function initFauxDevToolsHook() {
const onCommitFiberRoot = jest.fn();
const onCommitFiberUnmount = jest.fn();

let idCounter = 0;
const renderers = new Map();

// This is a minimal shim for the global hook installed by DevTools.
// The real one is in packages/react-devtools-shared/src/hook.js.
global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = {
renderers,
supportsFiber: true,
inject(renderer) {
const id = ++idCounter;
renderers.set(id, renderer);
return id;
},
onCommitFiberRoot,
onCommitFiberUnmount,
};
}

// This simulates the scenario in https://github.com/facebook/react/issues/17626
it('can inject the runtime after the renderer executes', () => {
if (__DEV__) {
// This is a minimal shim for the global hook installed by DevTools.
// The real one is in packages/react-devtools-shared/src/hook.js.
let idCounter = 0;
const renderers = new Map();
global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = {
renderers,
supportsFiber: true,
inject(renderer) {
const id = ++idCounter;
renderers.set(id, renderer);
return id;
},
onCommitFiberRoot() {},
onCommitFiberUnmount() {},
};
initFauxDevToolsHook();

// Load these first, as if they're coming from a CDN.
jest.resetModules();
Expand Down Expand Up @@ -3820,4 +3828,39 @@ describe('ReactFresh', () => {
expect(el.style.color).toBe('red');
}
});

// This simulates the scenario in https://github.com/facebook/react/issues/20100
it('does not block DevTools when an unsupported renderer is injected', () => {
if (__DEV__) {
initFauxDevToolsHook();

const onCommitFiberRoot =
global.__REACT_DEVTOOLS_GLOBAL_HOOK__.onCommitFiberRoot;

// Redirect all React/ReactDOM requires to v16.8.0
// This version predates Fast Refresh support.
jest.mock('react', () => jest.requireActual('react-16-8'));
jest.mock('react-dom', () => jest.requireActual('react-dom-16-8'));

// Load React and company.
jest.resetModules();
React = require('react');
ReactDOM = require('react-dom');
Scheduler = require('scheduler');

// Important! Inject into the global hook *after* ReactDOM runs:
ReactFreshRuntime = require('react-refresh/runtime');
ReactFreshRuntime.injectIntoGlobalHook(global);

render(() => {
function Hello() {
return <div>Hi!</div>;
}
$RefreshReg$(Hello, 'Hello');
return Hello;
});

expect(onCommitFiberRoot).toHaveBeenCalled();
}
});
});
28 changes: 28 additions & 0 deletions yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11499,6 +11499,16 @@ rc@^1.0.1, rc@^1.1.6, rc@^1.2.8:
object-assign "^4.1.0"
prop-types "^15.5.10"

"react-16-8@npm:[email protected]":
version "16.8.0"
resolved "https://registry.yarnpkg.com/react/-/react-16.8.0.tgz#8533f0e4af818f448a276eae71681d09e8dd970a"
integrity sha512-g+nikW2D48kqgWSPwNo0NH9tIGG3DsQFlrtrQ1kj6W77z5ahyIHG0w8kPpz4Sdj6gyLnz0lEd/xsjOoGge2MYQ==
dependencies:
loose-envify "^1.1.0"
object-assign "^4.1.1"
prop-types "^15.6.2"
scheduler "^0.13.0"

"react-dom-15@npm:react-dom@^15":
version "15.6.2"
resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-15.6.2.tgz#41cfadf693b757faf2708443a1d1fd5a02bef730"
Expand All @@ -11509,6 +11519,16 @@ rc@^1.0.1, rc@^1.1.6, rc@^1.2.8:
object-assign "^4.1.0"
prop-types "^15.5.10"

"react-dom-16-8@npm:[email protected]":
version "16.8.0"
resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-16.8.0.tgz#18f28d4be3571ed206672a267c66dd083145a9c4"
integrity sha512-dBzoAGYZpW9Yggp+CzBPC7q1HmWSeRc93DWrwbskmG1eHJWznZB/p0l/Sm+69leIGUS91AXPB/qB3WcPnKx8Sw==
dependencies:
loose-envify "^1.1.0"
object-assign "^4.1.1"
prop-types "^15.6.2"
scheduler "^0.13.0"

react-is@^16.12.0, react-is@^16.8.1:
version "16.13.1"
resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.13.1.tgz#789729a4dc36de2999dc156dd6c1d9c18cea56a4"
Expand Down Expand Up @@ -12315,6 +12335,14 @@ scheduler@^0.11.0:
loose-envify "^1.1.0"
object-assign "^4.1.1"

scheduler@^0.13.0:
version "0.13.6"
resolved "https://registry.yarnpkg.com/scheduler/-/scheduler-0.13.6.tgz#466a4ec332467b31a91b9bf74e5347072e4cd889"
integrity sha512-IWnObHt413ucAYKsD9J1QShUKkbKLQQHdxRyw73sw4FN26iWr3DY/H34xGPe4nmL1DwXyWmSWmMrA9TfQbE/XQ==
dependencies:
loose-envify "^1.1.0"
object-assign "^4.1.1"

schema-utils@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/schema-utils/-/schema-utils-1.0.0.tgz#0b79a93204d7b600d4b2850d1f66c2a34951c770"
Expand Down

0 comments on commit 343d7a4

Please sign in to comment.