## Summary
This commit is a proposal for handling duplicate installation of DevTools, in particular scoped to duplicates such as a dev build or the internal versions of DevTools installed alongside the Chrome Web Store extension.
Specifically, this commit makes it so when another instance of the DevTools extension is installed alongside the extension installed from the Chrome Web Store, we don't produce a stream of errors or crash Chrome, which is what would usually happen in this case.
### Detecting Duplicate Installations
- First, we check what type of installation the extension is: from the Chrome Web Store, the internal build of the extension, or a local development build.
- If the extension is from the **Chrome Web Store**:
- During initialization, we first check if the internal or local builds of the extension have already been installed and are enabled. To do this, we send a [cross-extension message](https://developer.chrome.com/docs/extensions/mv3/messaging/#external) to the internal and local builds of the extension using their extension IDs.
- We can do this because the extension ID for the internal build (and for the Chrome Web Store) is a stable ID.
- For the local build, at build time we hardcode a [`key` in the `manifest.json`](https://developer.chrome.com/docs/extensions/mv2/manifest/key/) which allows us to have a stable ID even for local builds.
- If we detect that the internal or local extensions are already installed, then we skip initializing the current extension altogether so as to not conflict with the other versions. This means we don't initialize the frontend or the backend at all.
- If the extension is the **Internal Build**:
- During initialization, we first check if the local builds of the extension has already been installed and is enabled. To do this, we send a [cross-extension message](https://developer.chrome.com/docs/extensions/mv3/messaging/#external) to the local build of the extension using its extension ID.
- We can do this for the local build because at build time we hardcode a [`key` in the `manifest.json`](https://developer.chrome.com/docs/extensions/mv2/manifest/key/) which allows us to have a stable ID even for local builds.
- If we detect that the local extension is already installed, then we skip initializing the current extension altogether so as to not conflict with the that version. This means we don't initialize the frontend or the backend at all.
- If the extension is a **Local Dev Build**:
- Since other extensions check for the existence of this extension and disable themselves if they detect it, we don't need any special handling during initialization and assume that there are no duplicate extensions. This means that we will generally prefer keeping this extension enabled.
This behavior means that the order of priority for keeping an extension enabled is the following:
1. Local build
2. Internal build
3. Public build
### Preventing duplicate backend initialization
Note that the backend is injected and initialized by the content script listening to a message posted to the inspected window (via `postMessage`). Since the content script will be injected twice, once each by each instance of the extension, even if we initialize the extension once, both content scripts would still receive the single message posted from the single frontend, and it would then still inject and initialize the backend twice.
In order to prevent this, we also add the extension ID to the message for injecting the backend. That way each content script can check if the message comes from its own extension, and if not it can ignore the message and avoid double injecting the backend.
### Other approaches
- I considered using the [`chrome.management`](https://developer.chrome.com/docs/extensions/reference/management/) API generally to detect other installations, but that requires adding additional permissions to our production extension, which didn't seem ideal.
- I also considered a few options of writing a special flag to the inspected window and checking for it before initializing the extension. However, it's hard to avoid race conditions in that case, and it seemed more reliable to check specifically for the WebStore extension, which is realistically where we would encounter the overlap.
### Rollout
- This commit needs to be published and rolled out to the Chrome Web Store first.
- After this commit is published to the Chrome Web Store, any duplicate instances of the extension that are built and installed after this commit will no longer conflict with the Chrome Web Store version.
### Next Steps
- In a subsequent PR, I will extend this code to show a warning when duplicate extensions have been detected.
Part of #22486
## How did you test this change?
### Basic Testing
- yarn flow
- yarn test
- yarn test-build-devtools
### Double installation testing
Testing double-installed extensions for this commit is tricky because we are relying on the extension ID of the internal and Chrome Web Store extensions, but we obviously can't actually test the Web Store version (since we can't modify the already published version).
In order to simulate duplicate extensions installed, I did the following process:
- Built separate extensions where I hardcoded a constant for whether the extension is internal or public (e.g. `EXTENSION_INSTALLATION_TYPE = 'internal'`). Then I installed these built extensions corresponding to the "internal" and "Web Store" builds.
- Build and run the regular development extension (with `yarn build:chrome:dev && yarn test:chrome`), using the extension IDs of the previously built extensions as the "internal" and "public" extension IDs.
With this set up in place, I tested the following on pages both with and without React:
- When only the local extension enabled, DevTools works normally.
- When only the "internal" extension enabled, DevTools works normally.
- When only the "public" extension enabled, DevTools works normally.
- When "internal" and "public" extensions are installed, "public" extension is disabled and "internal" extension works normally.
- When the local extension runs alongside the other extensions, other extensions disable themselves and local build works normally.
- When we can't recognize what type of build the extension corresponds to, we show an error.
- When all 3 extensions are installed and enabled in all different combinations, DevTools no longer produces errors or crashes Chrome, and works normally.
* Handle render phase updates explicitly
We fire a warning in development if a component is updated during
the render phase (with the exception of local hook updates, which have
their own defined behavior).
Because it's not a supported React pattern, we don't have that many
tests that trigger this path. But it is meant to have reasonable
semantics when it does happen, so that if it accidentally ships to
production, the app doesn't crash unnecessarily. The behavior is not
super well-defined, though.
There are also some _internal_ React implementation details that
intentionally to rely on this behavior. Most prominently, selective
hydration and useOpaqueIdentifier.
I need to tweak the behavior of render phase updates slightly as part
of a fix for useOpaqueIdentifier. This shouldn't cause a user-facing
change in behavior outside of useOpaqueIdentifier, but it does require
that we explicitly model render phase updates.
* Bugfix: Nested useOpaqueIdentifier calls
Fixes an issue where multiple useOpaqueIdentifier hooks are upgraded
to client ids within the same render.
The way the upgrade works is that useOpaqueIdentifier schedules a
render phase update then throws an error to trigger React's error
recovery mechanism.
The normal error recovery mechanism is designed for errors that occur
as a result of interleaved mutations, so we usually only retry a single
time, synchronously, before giving up.
useOpaqueIdentifier is different because the error its throws when
upgrading is not caused by an interleaved mutation. Rather, it happens
when an ID is referenced for the first time inside a client-rendered
tree (i.e. sommething that wasn't part of the initial server render).
The fact that it relies on the error recovery mechanism is an
implementation detail. And a single recovery attempt may be
insufficient. For example, if a parent and a child component may
reference different ids, and both are mounted as a result of the same
client update, that will trigger two separate error recovery attempts.
Because render phase updates are not allowed when triggered from
userspace — we log a warning in developement to prevent them —
we can assume that if something does update during the render phase, it
is one of our "legit" implementation details like useOpaqueIdentifier.
So we can keep retrying until we succeed — up to a limit, to protect
against inifite loops. I chose 50 since that's the limit we use for
commit phase updates.
* Rename pipeToNodeWritable to renderToNodePipe
* Add startWriting API to Flight
We don't really need it in this case because there's way less reason to
delay the stream in Flight.
* Pass the destination to startWriting instead of renderToNode
* Rename startWriting to pipe
This mirrors the ReadableStream API in Node
* Error codes
* Rename to renderToPipeableStream
This mimics the renderToReadableStream API for the browser.
* Hoist error codes import to module scope
When this code was written, the error codes map (`codes.json`) was
created on-the-fly, so we had to lazily require from inside the visitor.
Because `codes.json` is now checked into source, we can import it a
single time in module scope.
* Minify error constructors in production
We use a script to minify our error messages in production. Each message
is assigned an error code, defined in `scripts/error-codes/codes.json`.
Then our build script replaces the messages with a link to our
error decoder page, e.g. https://reactjs.org/docs/error-decoder.html/?invariant=92
This enables us to write helpful error messages without increasing the
bundle size.
Right now, the script only works for `invariant` calls. It does not work
if you throw an Error object. This is an old Facebookism that we don't
really need, other than the fact that our error minification script
relies on it.
So, I've updated the script to minify error constructors, too:
Input:
Error(`A ${adj} message that contains ${noun}`);
Output:
Error(formatProdErrorMessage(ERR_CODE, adj, noun));
It only works for constructors that are literally named Error, though we
could add support for other names, too.
As a next step, I will add a lint rule to enforce that errors written
this way must have a corresponding error code.
* Minify "no fallback UI specified" error in prod
This error message wasn't being minified because it doesn't use
invariant. The reason it didn't use invariant is because this particular
error is created without begin thrown — it doesn't need to be thrown
because it's located inside the error handling part of the runtime.
Now that the error minification script supports Error constructors, we
can minify it by assigning it a production error code in
`scripts/error-codes/codes.json`.
To support the use of Error constructors more generally, I will add a
lint rule that enforces each message has a corresponding error code.
* Lint rule to detect unminified errors
Adds a lint rule that detects when an Error constructor is used without
a corresponding production error code.
We already have this for `invariant`, but not for regular errors, i.e.
`throw new Error(msg)`. There's also nothing that enforces the use of
`invariant` besides convention.
There are some packages where we don't care to minify errors. These are
packages that run in environments where bundle size is not a concern,
like react-pg. I added an override in the ESLint config to ignore these.
* Temporarily add invariant codemod script
I'm adding this codemod to the repo temporarily, but I'll revert it
in the same PR. That way we don't have to check it in but it's still
accessible (via the PR) if we need it later.
* [Automated] Codemod invariant -> Error
This commit contains only automated changes:
npx jscodeshift -t scripts/codemod-invariant.js packages --ignore-pattern="node_modules/**/*"
yarn linc --fix
yarn prettier
I will do any manual touch ups in separate commits so they're easier
to review.
* Remove temporary codemod script
This reverts the codemod script and ESLint config I added temporarily
in order to perform the invariant codemod.
* Manual touch ups
A few manual changes I made after the codemod ran.
* Enable error code transform per package
Currently we're not consistent about which packages should have their
errors minified in production and which ones should.
This adds a field to the bundle configuration to control whether to
apply the transform. We should decide what the criteria is going
forward. I think it's probably a good idea to minify any package that
gets sent over the network. So yes to modules that run in the browser,
and no to modules that run on the server and during development only.
The `download-experimental-build` script has been flaky on CodeSandbox
CI, I think because of GitHub rate limiting.
Until we figure out how to fix that, I've updated it to fall back to a
local build.
* Pass in Destination lazily in startFlowing instead of createRequest
* Delay fatal errors until we have a destination to forward them to
* Flow can now be inferred by whether there's a destination set
We can drop the destination when we're not flowing since there's nothing to
write to.
Fatal errors now close once flowing starts back up again.
* Defer fatal errors in Flight too
As a follow up to #22445, this extracts the queueing logic that is
shared between `dispatchSetState` and `dispatchReducerAction` into
separate functions. It likely doesn't save any bytes since these will
get inlined, anyway, but it does make the flow a bit easier to follow.
* Remove reentrant check from Fizz/Flight
* Make startFlowing explicit in Flight
This is already an explicit call in Fizz. This moves flowing to be explicit.
That way we can avoid calling it in start() for web streams and therefore
avoid the reentrant call.
* Add regression test
This test doesn't actually error due to the streams polyfill not behaving
like Chrome but rather according to spec.
* Update the Web Streams polyfill
Not that we need this but just in case there are differences that are fixed.
* Fork dispatchAction for useState/useReducer
* Remove eager bailout from forked dispatchReducerAction, update tests
* Update eager reducer/state logic to handle state case only
* sync reconciler fork
* rename test
* test cases from #15198
* comments on new test cases
* comments on new test cases
* test case from #21419
* minor tweak to test name to kick CI
* Move flushSync warning to React DOM
When you call in `flushSync` from an effect, React fires a warning. I've
moved the implementation of this warning out of the reconciler and into
React DOM.
`flushSync` is a renderer API, not an isomorphic API, because it has
behavior that was designed specifically for the constraints of React
DOM. The equivalent API in a different renderer may not be the same.
For example, React Native has a different threading model than the
browser, so it might not make sense to expose a `flushSync` API to the
JavaScript thread.
* Make root.unmount() synchronous
When you unmount a root, the internal state that React stores on the
DOM node is immediately cleared. So, we should also synchronously
delete the React tree. You should be able to create a new root using
the same container.
Changes our text encoding approach to properly support multibyte characters following this algorithm. Based on benchmarking, this new approach is roughly equivalent in terms of performance (sometimes slightly faster, sometimes slightly slower).
I also considered using TextEncoder/TextDecoder for this, but it was much slower (~85%).
* Revise ESLint rules for string coercion
Currently, react uses `'' + value` to coerce mixed values to strings.
This code will throw for Temporal objects or symbols.
To make string-coercion safer and to improve user-facing error messages,
This commit adds a new ESLint rule called `safe-string-coercion`.
This rule has two modes: a production mode and a non-production mode.
* If the `isProductionUserAppCode` option is true, then `'' + value`
coercions are allowed (because they're faster, although they may
throw) and `String(value)` coercions are disallowed. Exception:
when building error messages or running DEV-only code in prod
files, `String()` should be used because it won't throw.
* If the `isProductionUserAppCode` option is false, then `'' + value`
coercions are disallowed (because they may throw, and in non-prod
code it's not worth the risk) and `String(value)` are allowed.
Production mode is used for all files which will be bundled with
developers' userland apps. Non-prod mode is used for all other React
code: tests, DEV blocks, devtools extension, etc.
In production mode, in addiiton to flagging `String(value)` calls,
the rule will also flag `'' + value` or `value + ''` coercions that may
throw. The rule is smart enough to silence itself in the following
"will never throw" cases:
* When the coercion is wrapped in a `typeof` test that restricts to safe
(non-symbol, non-object) types. Example:
if (typeof value === 'string' || typeof value === 'number') {
thisWontReport('' + value);
}
* When what's being coerced is a unary function result, because unary
functions never return an object or a symbol.
* When the coerced value is a commonly-used numeric identifier:
`i`, `idx`, or `lineNumber`.
* When the statement immeidately before the coercion is a DEV-only
call to a function from shared/CheckStringCoercion.js. This call is a
no-op in production, but in DEV it will show a console error
explaining the problem, then will throw right after a long explanatory
code comment so that debugger users will have an idea what's going on.
The check function call must be in the following format:
if (__DEV__) {
checkXxxxxStringCoercion(value);
};
Manually disabling the rule is usually not necessary because almost all
prod use of the `'' + value` pattern falls into one of the categories
above. But in the rare cases where the rule isn't smart enough to detect
safe usage (e.g. when a coercion is inside a nested ternary operator),
manually disabling the rule will be needed.
The rule should also be manually disabled in prod error handling code
where `String(value)` should be used for coercions, because it'd be
bad to throw while building an error message or stack trace!
The prod and non-prod modes have differentiated error messages to
explain how to do a proper coercion in that mode.
If a production check call is needed but is missing or incorrect
(e.g. not in a DEV block or not immediately before the coercion), then
a context-sensitive error message will be reported so that developers
can figure out what's wrong and how to fix the problem.
Because string coercions are now handled by the `safe-string-coercion`
rule, the `no-primitive-constructor` rule no longer flags `String()`
usage. It still flags `new String(value)` because that usage is almost
always a bug.
* Add DEV-only string coercion check functions
This commit adds DEV-only functions to check whether coercing
values to strings using the `'' + value` pattern will throw. If it will
throw, these functions will:
1. Display a console error with a friendly error message describing
the problem and the developer can fix it.
2. Perform the coercion, which will throw. Right before the line where
the throwing happens, there's a long code comment that will help
debugger users (or others looking at the exception call stack) figure
out what happened and how to fix the problem.
One of these check functions should be called before all string coercion
of user-provided values, except when the the coercion is guaranteed not
to throw, e.g.
* if inside a typeof check like `if (typeof value === 'string')`
* if coercing the result of a unary function like `+value` or `value++`
* if coercing a variable named in a whitelist of numeric identifiers:
`i`, `idx`, or `lineNumber`.
The new `safe-string-coercion` internal ESLint rule enforces that
these check functions are called when they are required.
Only use these check functions in production code that will be bundled
with user apps. For non-prod code (and for production error-handling
code), use `String(value)` instead which may be a little slower but will
never throw.
* Add failing tests for string coercion
Added failing tests to verify:
* That input, select, and textarea elements with value and defaultValue
set to Temporal-like objects which will throw when coerced to string
using the `'' + value` pattern.
* That text elements will throw for Temporal-like objects
* That dangerouslySetInnerHTML will *not* throw for Temporal-like
objects because this value is not cast to a string before passing to
the DOM.
* That keys that are Temporal-like objects will throw
All tests above validate the friendly error messages thrown.
* Use `String(value)` for coercion in non-prod files
This commit switches non-production code from `'' + value` (which
throws for Temporal objects and symbols) to instead use `String(value)`
which won't throw for these or other future plus-phobic types.
"Non-produciton code" includes anything not bundled into user apps:
* Tests and test utilities. Note that I didn't change legacy React
test fixtures because I assumed it was good for those files to
act just like old React, including coercion behavior.
* Build scripts
* Dev tools package - In addition to switching to `String`, I also
removed special-case code for coercing symbols which is now
unnecessary.
* Add DEV-only string coercion checks to prod files
This commit adds DEV-only function calls to to check if string coercion
using `'' + value` will throw, which it will if the value is a Temporal
object or a symbol because those types can't be added with `+`.
If it will throw, then in DEV these checks will show a console error
to help the user undertsand what went wrong and how to fix the
problem. After emitting the console error, the check functions will
retry the coercion which will throw with a call stack that's easy (or
at least easier!) to troubleshoot because the exception happens right
after a long comment explaining the issue. So whether the user is in
a debugger, looking at the browser console, or viewing the in-browser
DEV call stack, it should be easy to understand and fix the problem.
In most cases, the safe-string-coercion ESLint rule is smart enough to
detect when a coercion is safe. But in rare cases (e.g. when a coercion
is inside a ternary) this rule will have to be manually disabled.
This commit also switches error-handling code to use `String(value)`
for coercion, because it's bad to crash when you're trying to build
an error message or a call stack! Because `String()` is usually
disallowed by the `safe-string-coercion` ESLint rule in production
code, the rule must be disabled when `String()` is used.
If an error is thrown during a transition where we would have otherwise
suspended without showing a fallback (i.e. during a refresh), we should
still suspend.
The current behavior is that the error will force the fallback to
appear, even if it's completely unrelated to the component that errored,
which breaks the contract of `startTransition`.
Juan
Gabriel Trompiz
Brian Vaughn
Abhay Gupta
Joseph Savona
Andrew Clark
BIKI DAS
salazarm
Sebastian Markbåge
Daishi Kato
Ay-Ay-Ron
Luna Ruan
Behnam Mohammadi
Justin Grant