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] Add findSourceMapURL option to get a URL to load Server source maps from #29708

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Jun 2, 2024

This lets you click a stack frame on the client and see the Server source code inline.

Screenshot 2024-06-01 at 11 44 24 PM Screenshot 2024-06-01 at 11 43 37 PM

We could do some logic on the server that sends a source map url for every stack frame in the RSC payload. That would make the client potentially config free. However regardless we need the config to describe what url scheme to use since that’s not built in to the bundler config. In practice you likely have a common pattern for your source maps so no need to send data over and over when we can just have a simple function configured on the client.

The server must return a source map, even if the file is not actually compiled since the fake file is still compiled.

The source mapping strategy can be one of two models depending on if the server’s stack traces (new Error().stack) are source mapped back to the original (—enable-source-maps) or represents the location in compiled code (like in the browser).

If it represents the location in compiled code it’s actually easier. You just serve the source map generated for that file by the tooling.

If it is already source mapped it has to generate a source map where everything points to the same location (as if not compiled) ideally with a segment per logical ast node.

@sebmarkbage sebmarkbage requested review from gnoff and eps1lon June 2, 2024 03:47
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 3, 2024 2:54am

@react-sizebot
Copy link

react-sizebot commented Jun 2, 2024

Comparing: d77dd31...9f3f16c

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 = 496.48 kB 496.48 kB = 88.87 kB 88.87 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 501.30 kB 501.30 kB = 89.56 kB 89.56 kB
facebook-www/ReactDOM-prod.classic.js = 593.97 kB 593.97 kB = 104.48 kB 104.48 kB
facebook-www/ReactDOM-prod.modern.js = 570.35 kB 570.35 kB = 100.89 kB 100.89 kB
test_utils/ReactAllWarnings.js Deleted 63.65 kB 0.00 kB Deleted 15.90 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-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js +0.72% 101.67 kB 102.40 kB +0.78% 22.67 kB 22.85 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js +0.72% 101.91 kB 102.64 kB +0.78% 22.74 kB 22.91 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.development.js +0.69% 105.16 kB 105.89 kB +0.76% 23.71 kB 23.89 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js +0.69% 105.67 kB 106.40 kB +0.75% 23.87 kB 24.05 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js +0.67% 109.58 kB 110.31 kB +0.75% 24.82 kB 25.00 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.development.js +0.65% 111.37 kB 112.09 kB +0.73% 25.38 kB 25.57 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.65% 111.40 kB 112.13 kB +0.73% 25.40 kB 25.59 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.development.js +0.65% 112.80 kB 113.53 kB +0.72% 25.77 kB 25.95 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js +0.65% 112.83 kB 113.56 kB +0.72% 25.81 kB 26.00 kB
oss-experimental/react-client/cjs/react-client-flight.development.js +0.64% 101.66 kB 102.31 kB +0.76% 23.02 kB 23.20 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.development.js +0.64% 113.88 kB 114.61 kB +0.69% 25.98 kB 26.16 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js +0.64% 113.90 kB 114.63 kB +0.70% 26.02 kB 26.20 kB
oss-stable-semver/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js +0.26% 91.14 kB 91.37 kB +0.35% 19.96 kB 20.03 kB
oss-stable/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js +0.26% 91.14 kB 91.37 kB +0.35% 19.96 kB 20.03 kB
oss-stable-semver/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js +0.26% 91.38 kB 91.61 kB +0.35% 20.03 kB 20.10 kB
oss-stable/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js +0.26% 91.38 kB 91.61 kB +0.35% 20.03 kB 20.10 kB
oss-stable-semver/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.development.js +0.25% 94.63 kB 94.86 kB +0.34% 21.00 kB 21.07 kB
oss-stable/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.development.js +0.25% 94.63 kB 94.86 kB +0.34% 21.00 kB 21.07 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js +0.25% 95.14 kB 95.37 kB +0.34% 21.16 kB 21.23 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js +0.25% 95.14 kB 95.37 kB +0.34% 21.16 kB 21.23 kB
oss-stable-semver/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js +0.24% 99.05 kB 99.29 kB +0.34% 22.04 kB 22.11 kB
oss-stable/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js +0.24% 99.05 kB 99.29 kB +0.34% 22.04 kB 22.11 kB
oss-stable-semver/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.development.js +0.24% 100.83 kB 101.07 kB +0.33% 22.60 kB 22.67 kB
oss-stable/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.development.js +0.24% 100.83 kB 101.07 kB +0.33% 22.60 kB 22.67 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.23% 100.87 kB 101.10 kB +0.33% 22.62 kB 22.70 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.23% 100.87 kB 101.10 kB +0.33% 22.62 kB 22.70 kB
oss-stable-semver/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.development.js +0.23% 102.27 kB 102.50 kB +0.33% 22.98 kB 23.06 kB
oss-stable/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.development.js +0.23% 102.27 kB 102.50 kB +0.33% 22.98 kB 23.06 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js +0.23% 102.29 kB 102.53 kB +0.33% 23.03 kB 23.10 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js +0.23% 102.29 kB 102.53 kB +0.33% 23.03 kB 23.10 kB
oss-stable-semver/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.development.js +0.23% 103.34 kB 103.58 kB +0.30% 23.19 kB 23.26 kB
oss-stable/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.development.js +0.23% 103.34 kB 103.58 kB +0.30% 23.19 kB 23.26 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js +0.23% 103.37 kB 103.61 kB +0.31% 23.23 kB 23.30 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js +0.23% 103.37 kB 103.61 kB +0.31% 23.23 kB 23.30 kB
test_utils/ReactAllWarnings.js Deleted 63.65 kB 0.00 kB Deleted 15.90 kB 0.00 kB

Generated by 🚫 dangerJS against 9f3f16c

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.

This is amazing.

@@ -199,7 +199,7 @@ module.exports = function (webpackEnv) {
? shouldUseSourceMap
? 'source-map'
: false
: isEnvDevelopment && 'cheap-module-source-map',
: isEnvDevelopment && 'source-map',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a requirement for this feature to work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's just for client source maps. They end up off by one and not mapping to the right columns so not testing the proper experience.

fixtures/flight/server/region.js Outdated Show resolved Hide resolved
@@ -1721,9 +1731,13 @@ function createFakeFunction<T>(
}

const frameRegExp =
/^ {3} at (?:(.+) \(([^\)]+):(\d+):(\d+)\)|([^\)]+):(\d+):(\d+))$/;
/^ {3} at (?:(.+) \(([^\)]+):(\d+):(\d+)\)|(?:async )?([^\)]+):(\d+):(\d+))$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add one or two examples as comments here? Doesn't need to be super exhaustive. It's just a very intimidating regex 😬

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment but I'm still ambivalent about how much we should cover here. This doesn't really cover all possible.

// 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.
const sourceContent = await readFile(requestedFilePath, 'utf8');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now this throws when we request source map for node internal frames e.g. Error: ENOENT: no such file or directory, open 'node:internal/process/task_queues'.

Should we handle these on the server gracefully? Otherwise it might be a bit spammy. I guess the client can't handle it since it doesn't necessarily know what runtime internals are (assuming Bun uses other protocols).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally they're filtered before at the findSourceMapURL so that it can just return null instead.

This doesn't happen with just this PR though because we filter those out of the stack. It's not an issue until the next PR where it allows those to show up.

@sebmarkbage sebmarkbage merged commit ba099e4 into facebook:main Jun 3, 2024
40 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 3, 2024
…ce maps from (#29708)

This lets you click a stack frame on the client and see the Server
source code inline.

<img width="871" alt="Screenshot 2024-06-01 at 11 44 24 PM"
src="https://github.com/facebook/react/assets/63648/581281ce-0dce-40c0-a084-4a6d53ba1682">

<img width="840" alt="Screenshot 2024-06-01 at 11 43 37 PM"
src="https://github.com/facebook/react/assets/63648/00dc77af-07c1-4389-9ae0-cf1f45199efb">

We could do some logic on the server that sends a source map url for
every stack frame in the RSC payload. That would make the client
potentially config free. However regardless we need the config to
describe what url scheme to use since that’s not built in to the bundler
config. In practice you likely have a common pattern for your source
maps so no need to send data over and over when we can just have a
simple function configured on the client.

The server must return a source map, even if the file is not actually
compiled since the fake file is still compiled.

The source mapping strategy can be one of two models depending on if the
server’s stack traces (`new Error().stack`) are source mapped back to
the original (`—enable-source-maps`) or represents the location in
compiled code (like in the browser).

If it represents the location in compiled code it’s actually easier. You
just serve the source map generated for that file by the tooling.

If it is already source mapped it has to generate a source map where
everything points to the same location (as if not compiled) ideally with
a segment per logical ast node.

DiffTrain build for commit ba099e4.
github-actions bot pushed a commit that referenced this pull request Jun 3, 2024
…ce maps from (#29708)

This lets you click a stack frame on the client and see the Server
source code inline.

<img width="871" alt="Screenshot 2024-06-01 at 11 44 24 PM"
src="https://github.com/facebook/react/assets/63648/581281ce-0dce-40c0-a084-4a6d53ba1682">

<img width="840" alt="Screenshot 2024-06-01 at 11 43 37 PM"
src="https://github.com/facebook/react/assets/63648/00dc77af-07c1-4389-9ae0-cf1f45199efb">

We could do some logic on the server that sends a source map url for
every stack frame in the RSC payload. That would make the client
potentially config free. However regardless we need the config to
describe what url scheme to use since that’s not built in to the bundler
config. In practice you likely have a common pattern for your source
maps so no need to send data over and over when we can just have a
simple function configured on the client.

The server must return a source map, even if the file is not actually
compiled since the fake file is still compiled.

The source mapping strategy can be one of two models depending on if the
server’s stack traces (`new Error().stack`) are source mapped back to
the original (`—enable-source-maps`) or represents the location in
compiled code (like in the browser).

If it represents the location in compiled code it’s actually easier. You
just serve the source map generated for that file by the tooling.

If it is already source mapped it has to generate a source map where
everything points to the same location (as if not compiled) ideally with
a segment per logical ast node.

DiffTrain build for [ba099e4](ba099e4)
@sebmarkbage
Copy link
Collaborator Author

The craziest thing is that you can actually place break points inside the server code. It'll break in the eval that source maps to the server code. You can't see anything useful in that scope in terms of variables and you can't skip over any lines since it's just one callsite. It also doesn't actually prevent the server from running that code ofc. But you can navigate the stack easier in that way.

sebmarkbage added a commit that referenced this pull request Aug 18, 2024
This uses a similar technique to what we use to generate fake stack
frames for server components. This generates an eval:ed wrapper function
around the Server Reference proxy we create on the client. This wrapper
function gets the original `name` of the action on the server and I also
add a source map if `findSourceMapURL` is defined that points back to
the source of the server function.

For `"use server"` on the server, there's no new API. It just uses the
callsite of `registerServerReference()` on the Server. We can infer the
function name from the actual function on the server and we already have
the `findSourceMapURL` on the client receiving it.

For `"use server"` imported from the client, there's two new options
added to `createServerReference()` (in addition to the optional
[`encodeFormAction`](#27563)). These are only used in DEV mode. The
[`findSourceMapURL`](#29708) option is the same one added in #29708. We
need to pass this these references aren't created in the context of any
specific request but globally. The other weird thing about this case is
that this is actually a case where the compiled environment is the
client so any source maps are the same as for the client layer, so the
environment name here is just `"Client"`.

```diff
  createServerReference(
    id: string,
    callServer: CallServerCallback,
    encodeFormAction?: EncodeFormActionCallback,
+   findSourceMapURL?: FindSourceMapURLCallback, // DEV-only
+   functionName?: string, // DEV-only
  )
```

The key is that we use the location of the
`registerServerReference()`/`createServerReference()` call as the
location of the function. A compiler can either emit those at the same
locations as the original functions or use source maps to have those
segments refer to the original location of the function (or in the case
of a re-export the original location of the re-export is also a fine
approximate). The compiled output must call these directly without a
wrapper function because the wrapper adds a stack frame. I decided
against complicated and fragile dev-only options to skip n number of
frames that would just end up in prod code. The implementation just
skips one frame - our own. Otherwise it'll just point all source mapping
to the wrapper.

We don't have a `"use server"` imported from the client implementation
in the reference implementation/fixture so it's a bit tricky to test
that. In the case of CJS on the server, we just use a runtime instead of
compiler so it's tricky to source map those appropriately. We can
implement it for ESM on the server which is the main thing we're testing
in the fixture. It's easier in a real implementation where all the
compilation is just one pass. It's a little tricky since we have to
parse and append to other source maps but I'd like to do that as a
follow up. Or maybe that's just an exercise for the reader.

You can right click an action and click "Go to Definition".

<img width="1323" alt="Screenshot 2024-08-17 at 6 04 27 PM"
src="https://github.com/user-attachments/assets/94d379b3-8871-4671-a20d-cbf9cfbc2c6e">

For now they simply don't point to the right place but you can still
jump to the right file in the fixture:

<img width="1512" alt="Screenshot 2024-08-17 at 5 58 40 PM"
src="https://github.com/user-attachments/assets/1ea5d665-e25a-44ca-9515-481dd3c5c2fe">

In Firefox/Safari given that the location doesn't exist in the source
map yet, the browser refuses to open the file. Where as Chrome does
nearest (last) line.
sebmarkbage added a commit that referenced this pull request Aug 22, 2024
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