Component stacks have a similar problem to the problem with keyPath
where we had to move it down and set it late right before recursing.
Currently we work around that by popping exactly one off when something
suspends. That doesn't work with the new server stacks being added which
are more than one. It also meant that we kept having add a single frame
that could be popped when there shouldn't need to be one.
Unlike keyPath component stacks has this weird property that once
something throws we might need the stack that was attempted for errors
or the previous stack if we're going to retry and just recreate it.
I've tried a few different approaches and I didn't like either but this
is the one that seems least problematic.
I first split out renderNodeDestructive into a retryNode helper. During
retries only retryNode is called. When we first discover a node, we pass
through renderNodeDestructive.
Instead of add a component stack frame deep inside renderNodeDestructive
after we've already refined a node, we now add it before in
renderNodeDestructive. That way it's only added once before being
attempted. This is similar to how Fiber works where in ChildFiber we
match the node once to create the instance and then later do we attempt
to actually render it and it's only the second part that's ever retried.
This unfortunately means that we now have to refine the node down to
element/lazy/thenables twice. To avoid refining the type too I move that
to be done lazily.
We already added this for other thrown errors, not just console.errors.
There's a production form of this. We just missed adding this context.
Mainly the best context is the line number though which comes from owner
stacks.
Before:
<img width="844" alt="Screenshot 2024-07-04 at 3 20 34 PM"
src="https://github.com/facebook/react/assets/63648/0fd8a53f-538a-4429-a4cf-c22f85a09aa8">
After:
<img width="845" alt="Screenshot 2024-07-05 at 6 08 28 PM"
src="https://github.com/facebook/react/assets/63648/7b9da13a-fa97-4581-9899-06de6fface65">
Firefox:
<img width="1338" alt="Screenshot 2024-07-05 at 6 09 50 PM"
src="https://github.com/facebook/react/assets/63648/f2eb9f2a-2251-408f-86d0-b081279ba378">
The first log doesn't get a stack because it's logged before DevTools
boots up and connects which is unfortunate.
The second log already has a stack printed by React (this is on stable)
it gets replaced by our object now.
The third and following logs don't have a stack and get one appended.
I only turn the stack into an error object if it matches what we would
emit from DevTools anyway. Otherwise we assume it's not React. Since I
had to change the format slightly to make this work, I first normalize
the stack slightly before doing a comparison since it won't be 1:1.
This was missed in the mount dev dispatcher. It was only in the rerender
dispatcher which means that it was only logged during the rerender.
Since DevTools can hide logs during rerenders, this hid the warning in
StrictMode.
This is the same change as in #30289 but for the main runtime - e.g.
parent stacks in errorInfo.componentStack, appended stacks to
console.error coming from React itself and when we add virtual frames to
owner stacks.
Since we don't add location information these frames look weird to some
stack parsers - such as the native one. This is an existing issue when
you want to use some off-the-shelf parsers to parse production component
stacks for example.
While we won't add Error objects to logs ourselves necessarily, some
third party could want to do the same thing we do in DevTools and so we
should provide the same capability to just take this trace and print it
using an Error object.
Stacked on #30197.
This is similar to #30182 and #21610 in Fizz.
Track the current owner/stack/task on the task. This tracks it for
attribution when serializing child properties.
This lets us provide the right owner and createTask when we
console.error from inside Flight itself. This also affects the way we
print those logs on the client since we need the owner and stack. Now
console.errors that originate on the server gets the right stack on the
client:
<img width="760" alt="Screenshot 2024-07-03 at 6 03 13 PM"
src="https://github.com/facebook/react/assets/63648/913300f8-f364-4e66-a19d-362e8d776c64">
Unfortunately, because we don't track the stack we never pop it so it'll
keep tracking for serializing sibling properties. We rely on "children"
typically being the last property in the common case anyway. However,
this can lead to wrong attribution in some cases where the invalid
property is a next property (without a wrapping element) and there's a
previous element that doesn't. E.g. `<ClientComponent title={<div />}
invalid={nonSerializable} />` would use the div as the attribution
instead of ClientComponent.
I also wrap all of our own console.error, onError and onPostpone in the
context of the parent component. It's annoying to have to remember to do
this though.
We could always wrap the whole rendering in such as context but it would
add more overhead since this rarely actually happens. It might make
sense to track the whole current task instead to lower the overhead.
That's what we do in Fizz. We'd still have to remember to restore the
debug task though. I realize now Fizz doesn't do that neither so the
debug task isn't wrapping the console.errors that Fizz itself logs.
There's something off about that Flight and Fizz implementations don't
perfectly align.
Wire up owner stacks in Flight to the shared internals. This exposes it
to `captureOwnerStack()`.
In this case we install it permanently as we only allow one RSC renderer
which then supports async contexts. Same thing we do for owner.
This also ends up adding it to errors logged by React through
`consoleWithStackDev`. The plan is to eventually remove that but this is
inline with what we do in Fizz and Fiber already.
However, at the same time we've instrumented the console so we need to
strip them back out before sending to the client. This lets the client
control whether to add the stack back in or allowing
`console.createTask` to control it.
This is another reason we shouldn't append them from React but for now
we hack it by removing them after the fact.
Defaults to true in browser builds, otherwise defaults to false. The
assumption is that the server logs will already contain a log from the
original Flight server.
We currently always replay console logs but this leads to duplicates on
the server by default when you use SSR, because the Flight Client on the
server replays the logs. This can be nice since those logs gets badged.
It can also be nice if they're running in separate servers but when
they're logging to the same stream it's annoying. Which is really the
typical set up so we should just make that the default but leave it
configurable.
<!--
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
Fix how devtools handles URLs. It
- cannot handle relative source map URLs `//# sourceMappingURL=x.map`
- cannot recognize Windows style URLs
## How did you test this change?
works on my side
Use the same normalizeCodeLocInfo that we use everywhere else.
We should actually test the component stack itself. Not just that it
exists. This was causing false passes.
However, the logic was also wrong before because it wouldn't always
strip out the last line so wouldn't accurately normalize it. Leading to
false failures as well.
It can be efficient to accept raw string chunks to pass through a stream
instead of encoding them into a binary copy first.
Previously our Flight parsers didn't accept receiving string chunks.
That's partly because we sometimes need to encode binary chunks anyway
so string only transport isn't enough but some chunks can be strings.
This adds a partial ability for chunks to be received as strings.
However, accepting strings comes with some downsides. E.g. if the
strings are split up we need to buffer it which compromises the perf for
the common case. If the chunk represents binary data, then we'd need to
encode it back into a typed array which would require a TextEncoder
dependency in the parser. If the string chunk represents a byte length
encoded string we don't know how many unicode characters to read without
measuring them in terms of binary - also requiring a TextEncoder.
This PR is mainly intended for use for pass-through within the same
memory. We can simplify the implementation by assuming that any string
chunk is passed as the original chunk. This requires that the server
stream config doesn't arbitrarily concatenate strings (e.g. large
strings should not be concatenated which is probably a good heuristic
anyway). It also means that this is not suitable to be used with for
example receiving string chunks on the client by passing them through
SSR hydration data - except if the encoding that way was only used with
chunks that were already encoded as strings by Flight.
Web streams mostly just work on binary data anyway so they can't use
this.
In Node.js streams we concatenate precomputed and small strings into
larger buffers. It might make sense to do that using string ropes
instead. However, in the meantime we can at least pass large strings
that are outside our buffer view size as raw strings. There's no benefit
to us eagerly encoding those.
Also, let Node accept string chunks as long as they're following our
expected constraints. This lets us test the mixed protocol using
pass-throughs. This can also be useful when the RSC server is in the
same environment as the SSR server as they don't have to go from strings
to typed arrays back to strings.
Now we can also use this in the pass-through used in renderToMarkup.
This lets us avoid the dependency on TextDecoder/TextEncoder in that
package.
Stacked on #30170.
This lets us track Server Component parent stacks in Fizz which also
lets us track the correct owner stack for lazy.
In Fiber we're careful not to make any DEV only fibers but since the
ReactFizzComponentStack data structures just exist for debug meta data
anyway we can just expand on that.
Stacked on #30132.
This way we can get parent and owner stacks from the error.
This forces us to confront multiple errors and whether or not a Flight
error that ends up being unobservable needs to really reject the render.
This implements stashing of Flight errors with a digest and only errors
if they end up erroring the Fizz render too. At this point they'll have
parent stacks so we can surface those.
When we added component stacks to Fizz in prod it severely slowed down
common cases where you intentionally are throwing error for purposes of
client rendering. Our parent component stack generation is very slow
since call components with fake errors to generate them.
Therefore we disabled them in prod but included them in prerenders.
https://github.com/facebook/react/pull/27850
However, we still kept generating data structures for them and the code
still exists there for the prerenders. We could stop generating the data
structures which are not completely free but also not crazy bad.
What we can do instead is just lazily generate the component stacks.
This is in fact what plain stacks do anyway. This doesn't work as well
in Fiber because the data structures are live but on the server they're
immutable so it's fine to do it later as well.
That way you can choose to not read this getter for intentionally thrown
errors - after inspecting the Error object - yet still get component
stacks in prod for other errors.
Stacked on #30142.
This tracks owners and their stacks in DEV in Fizz. We use the
ComponentStackNode as the data structure to track this information -
effectively like ReactComponentInfo (Server) or Fiber (Client). They're
the instance.
I then port them same logic from ReactFiberComponentStack,
ReactFiberOwnerStack and ReactFiberCallUserSpace to Fizz equivalents.
This gets us both owner stacks from `captureOwnerStack()`, as well as
appended to console.errors logged by Fizz, while rendering and in
onError.
We use this to encode the binary length of a large string without
escaping it. This is really kind of optional though. This lets a Server
that can't encode strings but just pass them along able to emit RSC -
albeit a less optimal format.
The only build we have that does that today is react-html but the FB
version of Flight had a similar constraint.
It's still possible to support binary data as long as
byteLengthOfBinaryChunk is implemented which doesn't require a text
encoder. Many streams (including Node streams) support binary OR string
chunks.
Even though the whole package is private right now. Once we publish it,
it'll likely be just the experimental channel first before upgrading to
stable.
This means it gets excluded from the built packages.
Stacked on top of #30121.
This is the same thing we do for `renderToReadableStream` so that you
don't have to manually inject it into the stream.
The only reason we didn't for `renderToString` / `renderToStaticMarkup`
was to preserve legacy behavior but since this is a new API we can
change that.
If you're rendering a partial it doesn't matter. This is likely what
you'd do for RSS feeds. The question is if you can reliably rely on the
doctype being used while rendering e-mails since many clients are so
quirky. However, if you're careful it also doesn't hurt so it seems best
to include it.
Follow up to #30105.
This supports `renderToMarkup` in a non-RSC environment (not the
`react-server` condition).
This is just a Fizz renderer but it errors at runtime when you use
state, effects or event handlers that would require hydration - like the
RSC version would. (Except RSC can give early errors too.)
To do this I have to move the `react-html` builds to a new `markup`
dimension out of the `dom-legacy` dimension so that we can configure
this differently from `renderToString`/`renderToStaticMarkup`.
Eventually that dimension can go away though if deprecated. That also
helps us avoid dynamic configuration and we can just compile in the
right configuration so the split helps anyway.
One consideration is that if a compiler strips out useEffects or inlines
initial state from useState, then it would not get called an the error
wouldn't happen. Therefore to preserve semantics, a compiler would need
to inject some call that can check the current renderer and whether it
should throw.
There is an argument that it could be useful to not error for these
because it's possible to write components that works with SSR but are
just optionally hydrated. However, there's also an argument that doing
that silently is too easy to lead to mistakes and it's better to error -
especially for the e-mail use case where you can't take it back but you
can replay a queue that had failures. There are other ways to
conditionally branch components intentionally. Besides if you want it to
be silent you can still use renderToString (or better yet
renderToReadableStream).
The primary mechanism is the RSC environment and the client-environment
is really the secondary one that's only there to support legacy
environments. So this also ensures parity with the primary environment.
This is all behind the `enableOwnerStacks` flag.
This is a follow up to #29088. In that I moved type validation into the
renderer since that's the one that knows what types are allowed.
However, I only removed it from `React.createElement` and not the JSX
which was an oversight.
However, I also noticed that for invalid types we don't have the right
stack trace for throws because we're not yet inside the JSX element that
itself is invalid. We should use its stack for the stack trace. That's
the reason it's enough to just use the throw now because we can get a
good stack trace from the owner stack. This is fixed by creating a fake
Throw Fiber that gets assigned the right stack.
Additionally, I noticed that for certain invalid types like the most
common one `undefined` we error in Flight so a missing import in RSC
leads to a generic error. Instead of erroring on the Flight side we
should just let anything that's not a Server Component through to the
client and then let the Client renderer determine whether it's a valid
type or not. Since we now have owner stacks through the server too, this
will still be able to provide a good stack trace on the client that
points to the server in that case.
<img width="571" alt="Screenshot 2024-06-25 at 6 46 35 PM"
src="https://github.com/facebook/react/assets/63648/6812c24f-e274-4e09-b4de-21deda9ea1d4">
To get the best stack you have to expand the little icon and the regular
stack is noisy [due to this Chrome
bug](https://issues.chromium.org/issues/345248263) which makes it a
little harder to find but once that's fixed it might be easier.
Name of the package is tbd (straw: `react-html`). It's a new package
separate from `react-dom` though and can be used as a standalone package
- e.g. also from a React Native app.
```js
import {renderToMarkup} from '...';
const html = await renderToMarkup(<Component />);
```
The idea is that this is a helper for rendering HTML that is not
intended to be hydrated. It's primarily intended to support a subset of
HTML that can be used as embedding and not served as HTML documents from
HTTP. For example as e-mails or in RSS/Atom feeds or other
distributions. It's a successor to `renderToStaticMarkup`.
A few differences:
- This doesn't support "Client Components". It can only use the Server
Components subset. No useEffect, no useState etc. since it will never be
hydrated. Use of those are errors.
- You also can't pass Client References so you can't use components
marked with `"use client"`.
- Unlike `renderToStaticMarkup` this does support async so you can
suspend and use data from these components.
- Unlike `renderToReadableStream` this does not support streaming or
Suspense boundaries and any error rejects the promise. Since there's no
feasible way to "client render" or patch up the document.
- Form Actions are not supported since in an embedded environment
there's no place to post back to across versions. You can render plain
forms with fixed URLs though.
- You can't use any resource preloading like `preload()` from
`react-dom`.
## Implementation
This first version in this PR only supports Server Components since
that's the thing that doesn't have an existing API. Might add a Client
Components version later that errors.
We don't want to maintain a completely separate implementation for this
use case so this uses the `dom-legacy` build dimension to wire up a
build that encapsulates a Flight Server -> Flight Client -> Fizz stream
to render Server Components that then get SSR:ed.
There's no problem to use a Flight Client in a Server Component
environment since it's already supported for Server-to-Server. Both of
these use a bundler config that just errors for Client References though
since we don't need any bundling integration and this is just a
standalone package.
Running Fizz in a Server Component environment is a problem though
because it depends on "react" and it needs the client version.
Therefore, for this build we embed the client version of "react" shared
internals into the build. It doesn't need anything to be able to use
those APIs since you can't call the client APIs anyway.
One unfortunate thing though is that since Flight currently needs to go
to binary and back, we need TextEncoder/TextDecoder to be available but
this shouldn't really be necessary. Also since we use the legacy stream
config, large strings that use byteLengthOfChunk errors atm. This needs
to be fixed before shipping. I'm not sure what would be the best
layering though that isn't unnecessarily burdensome to maintain. Maybe
some kind of pass-through protocol that would also be useful in general
- e.g. when Fizz and Flight are in the same process.
---------
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
<!--
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
`Cannot access .then on server` is not an ideal message when you try to
await or do promise chain to the properties of client reference. The
below example will let `.then` get accessed by native code while
handling the promise chain but the access is not clearly visible in user
code.
```
import('./client-module').then((mod) => mod.Component)
```
This PR chnage the error message of module reference proxy '.then'
property to show more kinds of usage, then it can be pretty clearly for
helping users to avoid the bad usage
<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
## How did you test this change?
Unit test
<!--
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.
-->
When we replay logs we badge them with e.g. `[Server]`. That way it's
easy to identify that the source of the log actually happened on the
Server (RSC). However, when we threw an error we didn't have any such
thing. The error was rethrown on the client and then handled just like
any other client error.
This transfers the `environmentName` in DEV to our restored Error
"sub-class" (conceptually) along with `digest`. That way you can read
`error.environmentName` to print this in your own UI.
I also updated our default for `onCaughtError` (and `onError` in Fizz)
to use the `printToConsole` helper that the Flight Client uses to log it
with the badge format. So by default you get the same experience as
console.error for caught errors:
<img width="810" alt="Screenshot 2024-06-10 at 9 25 12 PM"
src="https://github.com/facebook/react/assets/63648/8490fedc-09f6-4286-9332-fbe6b0faa2d3">
<img width="815" alt="Screenshot 2024-06-10 at 9 39 30 PM"
src="https://github.com/facebook/react/assets/63648/bdcfc554-504a-4b1d-82bf-b717e74975ac">
Unfortunately I can't do the same thing for `onUncaughtError` nor
`onRecoverableError` because they use `reportError` which doesn't have
custom formatting (unless we also prevented default on window.onerror).
However maybe that's ok because 1) you should always have an error
boundary 2) it's not likely that an RSC error can actually recover
because it's not going to be rendered again so shouldn't really happen
outside some parent conditionally rendering maybe.
The other problem with this approach is that the default is no longer
trivial - so reimplementing the default in user space is trickier and
ideally we shouldn't expose our default to be called.
The explicit mock override in this test was causing it to always run as
native-oss instead of also as xplat. This moves the test to use `//
@gate persistent` instead to run it in all persistent configs.
## Summary
When DevTools frontend and backend are connected, we patch console in 2
places:
- `patch()`, when renderer is attached to:
- listen to any errors / warnings emitted
- append component stack if requested by the user
- `patchForStrictMode()`, when React notifies about that the next
invocation is about to happed during StrictMode
`patchForStrictMode()` will always be at the top of the patch stack,
because it is called at runtime when React notifies React DevTools,
because of this, `patch()` may receive already modified arguments (with
stylings for dimming), we should attempt to restore the original
arguments
## How did you test this change?
Look at yellow warnings on the element view:
| Before | After |
| --- | --- |
| 
| 
|
## Summary
Sometimes `constructor` happens to be the name of an unrelated property,
or we may be dealing with a `Proxy` that intercepts every read. Verify
the constructor is a function before using its name, and reset the name
anyway if it turns out not to be serializable.
Fixes some cases of the devtools crashing and becoming inoperable upon
attempting to inspect components whose props are Hookstate `State`s.
## How did you test this change?
Installed a patched version of the extension and confirmed that it
solves the problem.
---------
Co-authored-by: Ruslan Lesiutin <rdlesyutin@gmail.com>
## Summary
There is a race condition in the way we poll if React is on the page and
when we actually clear this polling instance. When user navigates to a
different page, we will debounce a callback for 500ms, which will:
1. Cleanup previous React polling instance
2. Start a new React polling instance
Since the cleanup logic is debounced, there is a small chance that by
the time we are going to clean up this polling instance, it will be
`eval`-ed on the page, that is using React. For example, when user is
navigating from the page which doesn't have React running, to a page
that has React running.
Next, we incorrectly will try to mount React DevTools panels twice,
which will result into conflicts in the Store, and the error will be
shown to the user
## How did you test this change?
Since this is a race condition, it is hard to reproduce consistently,
but you can try this flow:
1. Open a page that is using React, open browser DevTools and React
DevTools components panel
2. Open a page that is NOT using React, like google.com, wait ~5 seconds
until you see `"Looks like this page doesn't have React, or it hasn't
been loaded yet"` message in RDT panel
3. Open a page that is using React, observe the error `"Uncaught Error:
Cannot add node "1" because a node with that id is already in the
Store."`
Couldn't been able to reproduce this with these changes.
This PR adds parallelism similar to our existing circleci setup for
running yarn tests with the various test params. It does this by
sharding tests into `$SHARD_COUNT` number of groups, then spawning a job
for each of them and using jest's built in `--shard` option.
Effectively this means that the job will spawn an additional (where `n`
is the number of test params)
`n * $SHARD_COUNT` number of jobs to run tests in parallel
for a total of `n + (n * $SHARD_COUNT)` jobs. This does mean the
GitHub UI at the bottom of each PR gets longer and unfortunately it's
not sorted in any way as far as I can tell. But if something goes wrong
it should still be easy to find out what the problem is.
The PR also changes the `ci` argument for jest-cli to be an enum instead
so the tests use all available workers in GitHub actions. This will have
to live around for a bit until we can fully migrate off of circleci.
ghstack-source-id: 08f2d16353
Pull Request resolved: https://github.com/facebook/react/pull/30033
Need to tighten up this a bit.
react-dom isomorphic currently depends on react-reconciler which is
mostly DCE but it's pulled in which makes it hard to make other bundling
changes.
ReactFlightServer can have a hard dependency on the module that imports
its internals since even if other internals are aliased it still always
needs the server one.
Sebastian Markbåge
Jan Kassens
Sebastian Silbermann
Jack Works
Hendrik Liebau
Ruslan Lesiutin
Jiachi Liu
Timothy Yung
LoganDark
Jack Pope
Lauren Tan