Fixes https://github.com/facebook/react/issues/26500
## Summary
- No more using `clipboard-js` from the backend side, now emitting
custom `saveToClipboard` event, also adding corresponding listener in
`store.js`
- Not migrating to `navigator.clipboard` api yet, there were some issues
with using it on Chrome, will add more details to
https://github.com/facebook/react/pull/26539
## How did you test this change?
- Tested on Chrome, Firefox, Edge
- Tested on standalone electron app: seems like context menu is not
expected to work there (cannot right-click on value, the menu is not
appearing), other logic (pressing on copy icon) was not changed
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.
Before submitting a pull request, please make sure the following is
done:
1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
10. If you haven't already, complete the CLA.
Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->
## Summary
Addresses #26352.
This PR explicitly passes an icon to `chrome.devtools.panels.create()`,
so that edge devtools will display the icon when in [Focus
Mode](https://learn.microsoft.com/en-us/microsoft-edge/devtools-guide-chromium/experimental-features/focus-mode).
<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
## How did you test this change?
Passing test suite (`yarn test` & `yarn test --prod`) ✅
Passing lint (`yarn linc`) ✅
Passing type checks (`yarn flow`) ✅
**Visual Testing**
Before Changes | After Changes
:-------------------------:|:-------------------------:
|
<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
If you leave this empty, your PR will very likely be closed.
-->
## Summary
This pull request aims to improve the maintainability of the codebase by
consolidating types and constants that are shared between the backend
and frontend. This consolidation will allow us to maintain backwards
compatibility in the frontend in the future.
To achieve this, we have moved the shared types and constants to the
following blessed files:
- react-devtools-shared/src/constants
- react-devtools-shared/src/types
- react-devtools-shared/src/backend/types
- react-devtools-shared/src/backend/NativeStyleEditor/types
Please note that the inclusion of NativeStyleEditor in this list is
temporary, and we plan to remove it once we have a better plugin system
in place.
## How did you test this change?
I have tested it by running `yarn flow dom-node`, which reports no
errors.
## Summary
- #26234 is reverted and replaced with a better approach
- introduce a new global devtools variable to decouple the global hook's
dependency on backend/console.js, and add it to react-devtools-inline
and react-devtools-standalone
With this PR, I want to introduce a new principle to hook.js: we should
always be alert when editing this file and avoid importing from other
files.
In the past, we try to inline a lot of the implementation because we use
`.toString()` to inject this function from the extension (we still have
some old comments left). Although it is no longer inlined that way, it
has became now more important to keep it clean as it is a de facto
global API people are using (9.9K files contains it on Github search as
of today).
**File size change for extension:**
Before:
379K installHook.js
After:
21K installHook.js
363K renderer.js
## Summary
When looking into the compiled code of `installHook.js` of the extension
build, I noticed that it actually includes the large `attach` function
(from renderer.js). I don't think it was expected.
This is because `hook.js` imports from `backend/console.js` which
imports from `backend/renderer.js` for `getInternalReactConstants`
A straightforward way is to extract function
`getInternalReactConstants`. However, I think it's more simplified to
just merge these two files and save the 361K renderer.js from the
extension build since we have always been loading this code anyways.
I changed the execution check from `__REACT_DEVTOOLS_ATTACH__ ` to the
session storage.
## How did you test this change?
Everything works normal in my local build.
Fixes https://github.com/facebook/react/issues/25924 for React DevTools
specifically.
## Summary
If we remove the script after it's loaded, it creates a race condition
with other code. If some other code is searching for the first script
tag or first element of the document, this might broke it.
## How did you test this change?
I've tested in my local build that even if we remove the script tag
immediately, the code is still correctly executed.
## Summary
We had this as a temporary fix for #24626. Now that Chrome team decides
to turn the flag on again (with good reasons explained in
https://bugs.chromium.org/p/chromium/issues/detail?id=1241986#c31), we
will turn it into a long term solution.
In the future, we want to explore whether we can render React elements
on panel.html instead, as `requestAnimationFrame` produces higher
quality animation.
## How did you test this change?
Tested on local build with "Throttle non-visible cross-origin iframes"
flag enabled.
## Summary
Prettier was bumped recently. So any branch not including that bump,
might bring in outdated formatting (e.g.
https://github.com/facebook/react/pull/26068)
## How did you test this change?
- [x] `yarn prettier-all`
## Summary
This is to fix some edge cases I recently observed when developing and
using the extension:
- When you reload the page, there's a chance that a port (most likely
the devtools one) is not properly unloaded. In this case, the React
DevTools will stop working unless you create a new tab.
- For unknown reasons, Chrome sometimes spins up two service worker
processes. In this case, an error will be thrown "duplicate ID when
registering content script" and sometimes interrupt the execution of the
rest of service worker.
This is an attempt to make the logic more robust
- Automatically shutting down the double pipe if the message fails, and
allowing the runtime to rebuild the double pipe.
- Log the error message so Chrome believes we've handled it and will not
interrupt the execution.
This also seems to be helpful in fixing #25806.
The old version of prettier we were using didn't support the Flow syntax
to access properties in a type using `SomeType['prop']`. This updates
`prettier` and `rollup-plugin-prettier` to the latest versions.
I added the prettier config `arrowParens: "avoid"` to reduce the diff
size as the default has changed in Prettier 2.0. The largest amount of
changes comes from function expressions now having a space. This doesn't
have an option to preserve the old behavior, so we have to update this.
This setting is an incremental path to the next Flow version enforcing
type annotations on most functions (except some inline callbacks).
Used
```
node_modules/.bin/flow codemod annotate-functions-and-classes --write .
```
to add a majority of the types with some hand cleanup when for large
inferred objects that should just be `Fiber` or weird constructs
including `any`.
Suppressed the remaining issues.
Builds on #25918
## Summary
resolves#24522
To upgrade to Manifest V3, one of the biggest issue is that we are no
longer allowed to add a script element with code in textContent so that
it would run synchronously. It's necessary for us because we need to
inject a global hook for react reconciler to detect whether devtools
exist.
To do that, we'll leverage a new API
`chrome.scripting.registerContentScripts` in V3. Particularly, we rely
on the "world" option (added in Chrome v102
[commit](e5ad3451c1))
to run it in the "main world" on the page.
This PR also renames a few content script files so that it's easier to
tell them apart from other extension scripts and understand the purpose
of each of them.
Manifest V3 is not yet ready for Firefox, so we need to keep some code
for compatibility.
## How did you test this change?
`yarn build:chrome && yarn test:chrome`
`yarn build:edge && yarn test:edge`
`yarn build:firefox && yarn test:firefox`
Add column number for `viewSourceLineFunction` and renamed the function to `viewUrlSourceFunction` to match the other source function naming conventions
* [DevTools] front-end for profiling event stack
Adds a side-bar to the profiling tab. Users can now select an update event, and are
shown the callstack from the originating component. When a source path is available
there is now UI to jump to source.
Add FB enabled feature flag: enableProfilerComponentTree for the side-bar.
resolves#24170
* Revert "Only show DevTools warning about unrecognized build in Chrome (#22571)"
This reverts commit b72dc8e930.
* Revert "Show warning in UI when duplicate installations of DevTools extension are detected (#22563)"
This reverts commit 930c9e7eeb.
* Revert "Prevent errors/crashing when multiple installs of DevTools are present (#22517)"
This reverts commit 545d4c2de7.
* Remove all references to passing extensionId in postMessage
* Keep build changes
* lint
This commit adds code to all React bundles to explicitly register the beginning and ending of the module. This is done by creating Error objects (which capture the file name, line number, and column number) and passing them explicitly to a DevTools hook (when present).
Next, as the Scheduling Profiler logs metadata to the User Timing API, it prints these module ranges along with other metadata (like Lane values and profiler version number).
Lastly, the Scheduling Profiler UI compares stack frames to these ranges when drawing the flame graph and dims or de-emphasizes frames that fall within an internal module.
The net effect of this is that user code (and 3rd party code) stands out clearly in the flame graph while React internal modules are dimmed.
Internal module ranges are completely optional. Older profiling samples, or ones recorded without the React DevTools extension installed, will simply not dim the internal frames.
## 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.
* 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.
Replaced network.onRequestFinished() caching with network.getHAR() so that we can avoid redundantly (pre) caching JavaScript content. In the event that the HAR log doesn't contain a match, we'll fall back to fetching from the Network (and hoping for a cache hit from that layer).
I've tested both internally (internal Facebook DEV server) and externally (Code Sandbox) and it seems like this approach results in cache hits, so long as DevTools is opened when the page loads. (Otherwise it falls back to fetch().)
This commit builds on PR #22260 and makes the following changes:
* Adds a DevTools feature flag for named hooks support. (This allows us to disable it entirely for a build via feature flag.)
* Adds a new Suspense cache for dynamically imported modules. (This allows a component to suspend while importing an external code chunk– like the hook names parsing code).
* DevTools supports a hookNamesModuleLoaderFunction param to import the hook names module. I wish this could be handles as part of the react-devtools-shared package, but I'm not sure how to configure Webpack (4) to serve the chunk from react-devtools-inline. This seemed like a reasonable workaround.
The PR also contains an additional unrelated change:
* Removes pre-fetch optimization (added in DevTools: Improve named hooks network caching #22198). This optimization was mostly only important for cases where sources needed to be re-downloaded, something which we can now avoid in most cases¹ thanks to using cached responses already loaded by the page. (I tested this locally on Facebook and this change has no negative performance impact. There is still some overhead from serializing the JS through the Bridge but that's constant between the two approaches.)
¹ The case where we don't benefit from cached responses is when DevTools are opened after the page has already loaded certain scripts. This seems uncommon enough that I don't think it justified the added complexity of prefetching.
While testing the recently-launched named hooks feature, I noticed that one of the two big performance bottlenecks is fetching the source file. This was unexpected since the source file has already been loaded by the page. (After all, DevTools is inspecting a component defined in that same file.)
To address this, I made the following changes:
- [x] Keep CPU bound work (parsing source map and AST) in a worker so it doesn't block the main thread but move I/O bound code (fetching files) to the main thread.
- [x] Inject a function into the page (as part of the content script) to fetch cached files for the extension. Communicate with this function using `eval()` (to send it messages) and `chrome.runtime.sendMessage()` to return its responses to the extension).
With the above changes in place, the extension gets cached responses from a lot of sites- but not Facebook. This seems to be due to the following:
* Facebook's response headers include [`vary: 'Origin'`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Vary).
* The `fetch` made from the content script does not include an `Origin` request header.
To reduce the impact of cases where we can't re-use the Network cache, this PR also makes additional changes:
- [x] Use `devtools.network.onRequestFinished` to (pre)cache resources as the page loads them. This allows us to avoid requesting a resource that's already been loaded in most cases.
- [x] In case DevTools was opened _after_ some requests were made, we also now pre-fetch (and cache in memory) source files when a component is selected (if it has hooks). If the component's hooks are later evaluated, the source map will be faster to access. (Note that in many cases, this prefetch is very fast since it is returned from the disk cache.)
With the above changes, we've reduced the time spent in `loadSourceFiles` to nearly nothing.
