* 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.
Update all our local scripts to use `build` instead of `build2`.
There are still downstream scripts that depend on `build2`, though, so
we can't remove it yet.
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.
React currently suppress console logs in StrictMode during double rendering. However, this causes a lot of confusion. This PR moves the console suppression logic from React into React Devtools. Now by default, we no longer suppress console logs. Instead, we gray out the logs in console during double render. We also add a setting in React Devtools to allow developers to hide console logs during double render if they choose.
## Summary
Our current logic for extracting source map urls assumed that the url contained no query params (e.g. `?foo=bar`), and when extracting the url we would cut off the query params. I noticed this during internal testing, since removing the query params would cause loading source maps to fail.
This commit fixes that behavior by ensuring that our regex captures the full url, including query params.
## Test Plan
- yarn flow
- yarn test
- yarn test-build-devtools
- added new regression tests
- named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, internally).
## Summary
Before this commit, if a hook returned an array the was destructured, but without assigning a variable to the first element in the array, this would produce an error. This was detected via internal testing.
This commit fixes that and adds regression tests.
## Test Plan
- yarn flow
- yarn test
- yarn test-build-devtools
- added new regression tests
- named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, internally).
## Summary
Before this commit, if a hook returned an object and we declared a variable using object destructuring on the returned value, we would produce a runtime error. This was detected via internal testing.
This commit fixes that and adds regression tests.
## Test Plan
- yarn flow
- yarn test
- yarn test-build-devtools
- added new regression tests
- named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, internally).
## Summary
Follow up from https://github.com/facebook/react/pull/22010.
The initial implementation of named hooks and for looking up hook name metadata in an extended source map both assumed that the source maps would always have a `sources` field available, and didn't account for the source maps in the [Index Map](https://sourcemaps.info/spec.html#h.535es3xeprgt) format, which contain a list of `sections` and don't have the `source` field available directly.
In order to properly access metadata in extended source maps, this commit:
- Adds a new `SourceMapMetadataConsumer` api, which is a fork / very similar in structure to the corresponding [consumer in Metro](2b44ec39b4/packages/metro-symbolicate/src/SourceMetadataMapConsumer.js (L56)) (as specified by @motiz88 in https://github.com/facebook/react/issues/21782.
- Updates `parseHookNames` to use this new api
## Test Plan
- yarn flow
- yarn test
- yarn test-build-devtools
- added new regression tests covering the index map format
- named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, internally).
## Summary
Follow up from https://github.com/facebook/react/pull/22010.
As suggested by @motiz88, update the way the react sources metadata is stored within the fb sources metadata. Specifically, instead of `x_facebook_sources` directly containing a hook map in the second position of the metadata tuple for a given source, it contains the react sources metadata itself, which is also a tuple of react sources metadata for a given source, and which contains the hook map in the first position. This way the react sources metadata tuple can be extended to contain more react-specific metadata without taking up more positions in the top-level facebook sources metadata.
As part of this change:
- Adds more precise Flow types, mostly borrowed from Metro
- Fixes the facebook sources field name (we were using `x_fb_sources` but it should be `x_facebook_sources`
## Test Plan
- yarn flow
- yarn test
- yarn test-build-devtools
## Summary
Adds support for statically extracting names for hook calls from source code, and extending source maps with that information so that DevTools does not have to directly parse source code at runtime, which will speed up the Named Hooks feature and allow it to be enabled by default.
Specifically, this PR includes the following parts:
- [x] Adding logic to statically extract relevant hook names from the parsed source code (i.e. the babel ast). Note that this logic differs slightly from the existing logic in that the existing logic also uses runtime information from DevTools (such as whether given hooks are a custom hook) to extract names for hooks, whereas this code is meant to run entirely at build time, so it does not rely on that information.
- [x] Generating an encoded "hook map", which encodes the information about a hooks *original* source location, and it's corresponding name. This "hook map" will be used to generate extended source maps, included tentatively under an extra `x_react_hook_map` field. The map itself is formatted and encoded in a very similar way as how the `names` and `mappings` fields of a standard source map are encoded ( = Base64 VLQ delta coding representing offsets into a string array), and how the "function map" in Metro is encoded, as suggested in #21782. Note that this initial version uses a very basic format, and we are not implementing our own custom encoding, but reusing the `encode` function from `sourcemap-codec`.
- [x] Updating the logic in `parseHookNames` to check if the source maps have been extended with the hook map information, and if so use that information to extract the hook names without loading the original source code. In this PR we are manually generating extended source maps in our tests in order to test that this functionality works as expected, even though we are not actually generating the extended source maps in production.
The second stage of this work, which will likely need to occur outside this repo, is to update bundlers such as Metro to use these new primitives to actually generate source maps that DevTools can use.
### Follow-ups
- Enable named hooks by default when extended source maps are present
- Support looking up hook names when column numbers are not present in source map.
- Measure performance improvement of using extended source maps (manual testing suggests ~4 to 5x faster)
- Update relevant bundlers to generate extended source maps.
## Test Plan
- yarn flow
- Tests still pass
- yarn test
- yarn test-build-devtools
- Named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, facebook).
- For new functionality:
- New tests for statically extracting hook names.
- New tests for using extended source maps to look up hook names at runtime.
## Summary
This commit fixes an issue where DevTools would currently not correctly extract the hook names for a hook call when the hook call was nested under *another* hook call, e.g.:
```javascript
function Component() {
const InnerComponent = useMemo(() => () => {
const [state, setState] = useState(0);
return state;
});
return null;
};
```
Although it seems pretty rare to encounter this case in actual product code, DevTools wasn't handling it correctly:
**Expected Names:**
- `InnerComponent` for the `useMemo()` call.
- `state` for the `useState()` call.
**Actual**
- `InnerComponent` for the `useMemo()` call.
- `InnerComponent` for the `useState()` call.
The reason that we were extracting the name for the nested hook call incorrectly is that the `checkNodeLocation` function (which attempts to check if the location of the hook matches the location in the original source code), was too "lenient" and would return a match even if the start lines of the locations didn't match.
Specifically, for our example, it would consider that the location of the outer hook call "matched" the location of the inner hook call (even though they were on different lines), and would then return the wrong hook name.
### Fix
The fix in this commit is to update the `checkNodeLocation` function to more strictly check for matching start lines. The assumption here is that if 2 locations are on different starting lines, they can't possibly correspond to the same hook call.
## Test Plan
- yarn flow
- Tests still pass
- yarn test
- yarn test-build-devtools
- new regression tests added
- named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, internally).
## Summary
Adds a new unit test to `parseHookNames-test` which verifies that we correctly give names to hooks when they are used indirectly:
e.g.
```
const countState = useState(0);
const count = countState[0];
const setCount = countState[1];
```
Should produce `count` as the name.
## Test plan
```
yarn test
yarn test-build-devtools
yarn test-build-devtools parseHookNames
```
"cheap-module-source-map" is the default source-map generation mode used in created-react-dev mode because of speed. The major trade-off is that the source maps generated don't contain column numbers, so DevTools needs to be more lenient when matching AST nodes in this mode.
In this case, it can ignore column numbers and match nodes using line numbers only– so long as only a single node matches. If more than one match is found, treat it the same as if none were found, and fall back to no name.
