* 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`.
* Refactor throwException control flow
I'm about to add more branches to the Suspense-related logic in
`throwException`, so before I do, I split some of the steps into
separate functions so that later I can use them in multiple places.
This commit does not change any program behavior, only the control flow
surrounding existing code.
* Hydration errors should force a client render
If something errors during hydration, we should try rendering again
without hydrating.
We'll find the nearest Suspense boundary and force it to client render,
discarding the server-rendered content.
I think this naming is a bit clearer. It means the root is currently
showing server rendered content that needs to be hydrated.
A dehydrated root is conceptually the same as what we call dehydrated
Suspense boundary, so this makes the naming of the root align with the
naming of subtrees.
The scheduling profiler markComponentRenderStopped method is supposed to be called when rendering finishes or when a value is thrown (Suspense or Error). Previously we were calling this in a Suspense-only path of `throwException`.
This PR updates the code to handle errors (or non-Thenables) thrown as well.
It also moves the mark logic the work loop `handleError` method, with Suspense/Error agnostic cleanup.
Indexed maps divide nested source maps into sections, annotated with a line and column offset. Since these sections are JSON and can be quickly parsed, we can easily separate them without doing the heavier base64 and VLQ decoding process. This PR updates our sourcemap parsing code to defer parsing of an indexed map section until we actually need to retrieve mappings from it.
* Remove pushEmpty
This is only used to support the assignID mechanism.
* Remove assignID mechanism
This effectively isn't used anyway because we always insert a dummy tag
into the fallback.
* Emit the template tag with an ID directly in pending boundaries
This ensures that assigning the ID is deterministic since it's done during
writing.
This also avoids emitting it for client rendered boundaries that start as
client rendered since we never need to refer to them.
* Move lazy ID initialization to the core implementation
We never need an ID before we write a pending boundary. This also ensures
that ID generation is deterministic by moving it to the write phase.
* Simplify the inserted scripts
We can assume that there are no text nodes before the template tag so this
simplifies the script that finds the comment node. It should be the direct
previous child.
I noticed a weird branch where we attach a ping listener even in legacy
mode. It's weird because this shouldn't be necessary. Fallbacks always
synchronously commit in legacy mode, so pings never happen. (A "ping" is
when a suspended promise resolves before the fallback has committed.)
It took me a moment to remember why this case exists, but it's related
to React.lazy.
There's a special case where we suspend while reconciling the children
of a Suspense boundary's inner Offscreen wrapper fiber. This happens
when a React.lazy component is a direct child of a Suspense boundary.
Suspense boundaries are implemented as multiple fibers, but they are a
single conceptual unit. The legacy mode behavior where we pretend the
suspended fiber committed as `null` won't work, because in this case the
"suspended" fiber is the inner Offscreen wrapper.
Because the contents of the boundary haven't started rendering yet (i.e.
nothing in the tree has partially rendered) we can switch to the
regular, concurrent mode behavior: mark the boundary with ShouldCapture
and enter the unwind phase.
However, even though we're switching to the concurrent mode behavior, we
don't need to attach a ping listener. So I refactored the logic so that
it doesn't escape back into the regular path.
It's not really a big deal that we attach an unncessary ping listener,
since this case is so unusual. The motivation is not performance related
— it's to make the logic clearer, because I'm about to add another case
where we trigger a Suspense boundary without attaching a ping listener.
This commit dramatically improves the performance of the hook names feature by replacing the source-map-js integration with custom mapping code built on top of sourcemap-codec. Based on my own benchmarking, this makes parsing 3-4 times faster. (The bulk of these changes are in SourceMapConsumer.js.)
While implementing this code, I also uncovered a problem with the way we were caching source-map metadata that was causing us to potential parse the same source-map multiple times. (I addressed this in a separate commit for easier reviewing. The bulk of these changes are in parseSourceAndMetadata.js.)
Altogether these changes dramatically improve the performance of the hooks parsing code.
One additional thing we could look into if the source-map download still remains a large bottleneck would be to stream it and decode the mappings array while it streams in rather than in one synchronous chunk after the full source-map has been downloaded.
Going to revert this until we figure out error reporting. It looks like
our downstream infra already supports some type of error recovery so
we might not need it here.
We're still in the process of migrating to the Fizz server renderer. In
the meantime, this makes the error semantics on the old server renderer
match the behavior of the new one: if an error is thrown, it triggers a
Suspense fallback, just as if it suspended (this part was already
implemented). Then the errored tree is retried on the client, where it
may recover and finish successfully.
Recoil uses useMutableSource behind a flag. I thought this was fine
because Recoil isn't used in any concurrent roots, so the behavior
would be the same, but it turns out that it is used by concurrent
roots in a few places.
I'm not expecting it to be hard to migrate to useSyncExternalStore, but
to de-risk the change I'm going to roll it out gradually with a flag. In
the meantime, I've added back the useMutableSource API.
There's a downstream workflow that runs the `print-warnings` command. We
can make it faster by scraping the warnings in CI and storing the
result as a build artifact.
The publish-preleases command prints the URL of the publish workflow
so that you can visit the page and follow along.
But it can take a few seconds before the workflow ID is available, after
you create the pipeline. So the script polls the workflow endpoint
until it's available.
The current polling limit is too low so I increased it.
I also updated the error message to provide more info.
Previously, DevTools always overrode the native console to dim or supress StrictMode double logging. It also overrode console.log (in addition to console.error and console.warn). However, this changes the location shown by the browser console, which causes a bad developer experience. There is currently a TC39 proposal that would allow us to extend console without breaking developer experience, but in the meantime this PR changes the StrictMode console override behavior so that we only patch the console during the StrictMode double render so that, during the first render, the location points to developer code rather than our DevTools console code.
This removes all the remaining references to the `build2` directory
except for the CI job that stores the artifacts. We'll keep the
`build2` artifact until downstream scripts are migrated to `build`.
