Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Flight] Run recreated Errors within a fake native stack #29717

Merged
merged 5 commits into from
Jun 7, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Jun 2, 2024

Stacked on #29740.

Before:

Screenshot 2024-06-02 at 11 51 20 AM

After (updated with my patches to Chrome):

Screenshot 2024-06-06 at 5 16 20 PM

Sources panel after:

Screenshot 2024-06-06 at 5 14 21 PM

The fake eval file is now under "React" and the real file is now under file://

@sebmarkbage sebmarkbage requested review from eps1lon and hoxyq June 2, 2024 16:03
Copy link

vercel bot commented Jun 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2024 9:11pm

@react-sizebot
Copy link

react-sizebot commented Jun 2, 2024

Comparing: 3730b40...4ba415e

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.26 kB 497.26 kB = 89.12 kB 89.11 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.08 kB 502.08 kB = 89.80 kB 89.80 kB
facebook-www/ReactDOM-prod.classic.js = 594.56 kB 594.56 kB = 104.72 kB 104.72 kB
facebook-www/ReactDOM-prod.modern.js = 570.95 kB 570.95 kB = 101.13 kB 101.13 kB
test_utils/ReactAllWarnings.js Deleted 63.89 kB 0.00 kB Deleted 15.97 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-client/cjs/react-client-flight.development.js +1.16% 102.37 kB 103.56 kB +1.75% 23.22 kB 23.62 kB
oss-experimental/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js +1.16% 102.46 kB 103.65 kB +1.81% 22.87 kB 23.29 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js +1.16% 102.70 kB 103.89 kB +1.82% 22.94 kB 23.35 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.development.js +1.12% 105.95 kB 107.14 kB +1.74% 23.90 kB 24.32 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js +1.12% 106.46 kB 107.65 kB +1.74% 24.07 kB 24.48 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js +1.08% 110.37 kB 111.56 kB +1.62% 25.03 kB 25.44 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.development.js +1.06% 112.16 kB 113.35 kB +1.58% 25.59 kB 25.99 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +1.06% 112.19 kB 113.38 kB +1.59% 25.61 kB 26.02 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.development.js +1.05% 113.59 kB 114.78 kB +1.57% 25.97 kB 26.38 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js +1.05% 113.62 kB 114.81 kB +1.56% 26.02 kB 26.42 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.development.js +1.04% 114.67 kB 115.86 kB +1.56% 26.18 kB 26.59 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js +1.04% 114.70 kB 115.89 kB +1.55% 26.22 kB 26.63 kB
oss-stable-rc/react-client/cjs/react-client-flight.development.js +0.25% 91.35 kB 91.58 kB +0.22% 20.34 kB 20.38 kB
oss-stable-semver/react-client/cjs/react-client-flight.development.js +0.25% 91.35 kB 91.58 kB +0.22% 20.34 kB 20.38 kB
oss-stable/react-client/cjs/react-client-flight.development.js +0.25% 91.35 kB 91.58 kB +0.22% 20.34 kB 20.38 kB
oss-stable-rc/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js +0.25% 91.44 kB 91.67 kB +0.21% 20.06 kB 20.10 kB
oss-stable-semver/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js +0.25% 91.44 kB 91.67 kB +0.21% 20.06 kB 20.10 kB
oss-stable/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js +0.25% 91.44 kB 91.67 kB +0.21% 20.06 kB 20.10 kB
oss-stable-rc/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js +0.25% 91.68 kB 91.91 kB +0.21% 20.12 kB 20.17 kB
oss-stable-semver/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js +0.25% 91.68 kB 91.91 kB +0.21% 20.12 kB 20.17 kB
oss-stable/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js +0.25% 91.68 kB 91.91 kB +0.21% 20.12 kB 20.17 kB
oss-stable-rc/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.development.js +0.24% 94.93 kB 95.16 kB +0.20% 21.09 kB 21.13 kB
oss-stable-semver/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.development.js +0.24% 94.93 kB 95.16 kB +0.20% 21.09 kB 21.13 kB
oss-stable/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.development.js +0.24% 94.93 kB 95.16 kB +0.20% 21.09 kB 21.13 kB
oss-stable-rc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js +0.24% 95.44 kB 95.67 kB +0.20% 21.25 kB 21.29 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js +0.24% 95.44 kB 95.67 kB +0.20% 21.25 kB 21.29 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js +0.24% 95.44 kB 95.67 kB +0.20% 21.25 kB 21.29 kB
oss-stable-rc/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js +0.23% 99.35 kB 99.58 kB +0.20% 22.14 kB 22.18 kB
oss-stable-semver/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js +0.23% 99.35 kB 99.58 kB +0.20% 22.14 kB 22.18 kB
oss-stable/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js +0.23% 99.35 kB 99.58 kB +0.20% 22.14 kB 22.18 kB
oss-stable-rc/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.development.js +0.23% 101.13 kB 101.36 kB +0.19% 22.70 kB 22.74 kB
oss-stable-semver/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.development.js +0.23% 101.13 kB 101.36 kB +0.19% 22.70 kB 22.74 kB
oss-stable/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.development.js +0.23% 101.13 kB 101.36 kB +0.19% 22.70 kB 22.74 kB
oss-stable-rc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.23% 101.16 kB 101.40 kB +0.19% 22.72 kB 22.76 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.23% 101.16 kB 101.40 kB +0.19% 22.72 kB 22.76 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.23% 101.16 kB 101.40 kB +0.19% 22.72 kB 22.76 kB
oss-stable-rc/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.development.js +0.23% 102.57 kB 102.80 kB +0.19% 23.08 kB 23.12 kB
oss-stable-semver/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.development.js +0.23% 102.57 kB 102.80 kB +0.19% 23.08 kB 23.12 kB
oss-stable/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.development.js +0.23% 102.57 kB 102.80 kB +0.19% 23.08 kB 23.12 kB
oss-stable-rc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js +0.23% 102.59 kB 102.82 kB +0.19% 23.12 kB 23.16 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js +0.23% 102.59 kB 102.82 kB +0.19% 23.12 kB 23.16 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js +0.23% 102.59 kB 102.82 kB +0.19% 23.12 kB 23.16 kB
oss-stable-rc/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.development.js +0.22% 103.64 kB 103.88 kB +0.19% 23.27 kB 23.32 kB
oss-stable-semver/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.development.js +0.22% 103.64 kB 103.88 kB +0.19% 23.27 kB 23.32 kB
oss-stable/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.development.js +0.22% 103.64 kB 103.88 kB +0.19% 23.27 kB 23.32 kB
oss-stable-rc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js +0.22% 103.67 kB 103.90 kB +0.19% 23.32 kB 23.36 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js +0.22% 103.67 kB 103.90 kB +0.19% 23.32 kB 23.36 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js +0.22% 103.67 kB 103.90 kB +0.19% 23.32 kB 23.36 kB
test_utils/ReactAllWarnings.js Deleted 63.89 kB 0.00 kB Deleted 15.97 kB 0.00 kB

Generated by 🚫 dangerJS against 4ba415e

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 2, 2024

What's the difference? Is it that locations in the before open the file in a new tab while the added frames provide locations that are viewable in devtools?

It doesn't work for me for some reason. I checked out your branch and did a fresh build. I'm still seeing the same as before:
Screenshot 2024-06-02 at 23 14 44

I

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Jun 3, 2024

It's in the expanded stack if you expand the little triangle. The native stack now includes the server stack.

It's kind of annoying but when Chrome prints an Error object in console.error or reportError it prints the .stack property more or less as is - which means it's not source mapped and pretty printed links. It's probably better to use a custom onCaughtError/onUncaughtError that prints console.error(error.message) instead. A problem with console.error (which also applies to the default onCaughtError) is that the stack will be the stack of the component that errored, not the stack of the error itself. Unlike reportError which uses the native stack of the error itself.

@sebmarkbage
Copy link
Collaborator Author

This one is currently not filtering out internal frames as mentioned here: #29708 (comment)

Another thing that's new is that this is the first time we are exposing a stack trace that has been previously exposed to user space. So it could've been messed with and we aren't guaranteed to be able to use Error.prepareStackTrace on it.

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 3, 2024

Seeing it now. Though the mapping is off for me with FAST_REFRESH=false yarn dev
Screenshot 2024-06-03 at 15 11 13

The other stackframes are mapped correctly. Only these are incorrect:

getServerState @ ServerState.js:10
App @ App.js:49

Unfortunate that you have to have devtools open while the error happens. If you open devtools afterwards, you can't expand the error.

Aside: I can't copy something from the expanded error even though it's selectable
Screenshot 2024-06-03 at 15 17 30

@sebmarkbage
Copy link
Collaborator Author

They seem receptive and quick to respond to contributions so that might be something you can just fix with some CSS.

@sebmarkbage
Copy link
Collaborator Author

I can't repro any change with FAST_REFRESH=false yarn dev. It would be weird since there's no FAST_REFRESH specific things in the region service and the source maps is fully on that server and not the SSR or client that has the Webpack pieces. Unless some other library reads that env variable.

Could you have left anything in the cache? Sometimes I have to clear node_modules/.cache and recompile a few times.

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 3, 2024

The FAST_REFRESH is probably unrelated. But otherwise I can't run dev reliably. but that's an old issue.

I'll try it on a new worktree though I already cleared n_m/.cache.

// stacks will have had the source map already applied so it's pointing to the
// original source. We return a blank source map that just maps everything to
// the original source in this case.
if (requestedFilePath.startsWith('node:')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could filter out node internals on the server for thrown errors like we do for other owner stacks.

However, the rationale for filtering those out is mainly to save on bandwidth and eval() cost for stacks that are eagerly generated even if there is no error. If it wasn't for that it's generally up to the client to filter these out.

For errors that have actually thrown, that's less of a concern and it's nice to have the additional information if needed.

To make these automatically filtered out we can generate a source map for the internals with the ignoreList specified so that it's hidden by default but can still be expanded if you need it for some reason.

Unfortunately ignore lists aren't applied to the default printing of reportError/console.error(error) but that's a general problem that also applies to client infra stacks.

);
const rootTask = response._debugRootTask;
if (rootTask != null) {
error = rootTask.run(callStack);
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually have any effect and even if it did it's not the actual component that threw. The async task doesn't get captured by the error. That might be a bug in console.createTask. The async task that's applied is whatever task that reportError runs in - which is whatever client component this threw inside of (<Shell> in this case).

We could potentially restore the task of the server component that errored somehow before calling onUncaughtError such as if the source of the error was a React.lazy. We could also potentially annotate the error object with the server component's task. Currently we don't track which component originally threw the error.

This hides the node internals automatically but still lets them be visible
when expanded.
This lets Firefox find them relatively to node: URLs
If we always add sourceURL even when we have source maps, the native stack gets source mapped. This is needed when looking up a source location from an error string where the engine doesn't have access to the native reference.

Async stacks are printed with native references but errors printed into the console are printed as strings and then reverse engineered back into source
mapped urls. This preserves each eval as its own id.

We use a separate protocol to exclude them from other Sources. Otherwise the real file and the fake file are listed right next to eachother.
This way Chrome knows where this is on disk and that it's not a relative
path to the page's domain.
@sebmarkbage
Copy link
Collaborator Author

The nice thing about this approach is that we no longer override the stack but use the native stack. Even without createTask. So for example Firefox gets its native stack formatting instead of the server's format:

Screenshot 2024-06-06 at 5 48 48 PM

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They seem receptive and quick to respond to contributions so that might be something you can just fix with some CSS.

I see when I have the patience to clone that repo 😄

Mapping is correct now for me. Not sure why it didn't work previously.

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 7, 2024

They seem receptive and quick to respond to contributions so that might be something you can just fix with some CSS.

Maybe later. They seem to intentionally cut this off in event handlers though. Filed https://issues.chromium.org/u/1/issues/345518624 in the meantime to get clarification.

@sebmarkbage sebmarkbage merged commit cc1ec60 into facebook:main Jun 7, 2024
44 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 7, 2024
Stacked on #29740.

Before:

<img width="719" alt="Screenshot 2024-06-02 at 11 51 20 AM"
src="https://github.com/facebook/react/assets/63648/8f79fa82-2474-4583-894e-a2329e9a6304">

After (updated with my patches to Chrome):

<img width="813" alt="Screenshot 2024-06-06 at 5 16 20 PM"
src="https://github.com/facebook/react/assets/63648/bcc4f52f-e0ac-4708-ac2b-9629acdff705">

Sources panel after:

<img width="1188" alt="Screenshot 2024-06-06 at 5 14 21 PM"
src="https://github.com/facebook/react/assets/63648/2c673fac-d32d-42e4-8fac-bb63704e4b7f">

The fake eval file is now under "React" and the real file is now under
`file://`

DiffTrain build for commit cc1ec60.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants