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

Reliable JavaScript/SourceMap processing via DebugId #81

Merged
merged 5 commits into from
Apr 13, 2023

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Mar 21, 2023

We want to make processing / SourceMap-ing of JavaScript stack traces more reliable.
To achieve this, we want to uniquely identify a (minified / deployed) JavaScript file using a DebugId.
The same DebugId also uniquely identifies the corresponding SourceMap.
That way it should be possible to reliably look up the SourceMap corresponding to
a JavaScript file.

Rendered RFC

@Swatinem Swatinem changed the title Be able to look up SourceMaps by DebugId Reliable JavaScript/SourceMap lookup via DebugId Mar 22, 2023
@mitsuhiko
Copy link
Member

Ad unresolved questions: I think the debug_id reference in the sourcemap is something that makes quite a bit of sense to change.

@Swatinem Swatinem changed the title Reliable JavaScript/SourceMap lookup via DebugId Reliable JavaScript/SourceMap processing via DebugId Mar 22, 2023
@Swatinem Swatinem marked this pull request as ready for review March 23, 2023 13:17
@iambriccardo
Copy link
Member

Good work @Swatinem on the RFC, I am a bit confused by the cons of the Add the DebugId to a global at load time. Is it a big con the parsing of the Error.stack? To me, considering the other options, it is more of a pro, unless it is computationally expensive but on this I don't know.


**cons**

- Might incur some async fetching / IO when capturing an Error. Though any `abs_path` in the stack trace should be cached already.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't sound like a good idea to me. Mostly gut feeling but some other things come to mind:

  • Resource could be large
  • There might be delay in fetching the resource causing racing situations with navigations and so on
  • (I'll add additional reasons here when they come to mind)

text/0081-sourcemap-debugid.md Show resolved Hide resolved
- It does however require parsing of the `Error.stack` at time of capturing the `Error`.

An alternative implementation might use the [`import.meta.url`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import.meta)
property. This would avoid capturing and post-processing an `Error.stack`, but does require usage of ECMAScript Modules.
Copy link
Member

Choose a reason for hiding this comment

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

but does require usage of ECMAScript Modules

IMO this is a deal breaker. At least while IE is still somewhat relevant.

Choose a reason for hiding this comment

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

I'd rather build for the future, as when this spec become widespread the IE will be no more.

Copy link
Member

Choose a reason for hiding this comment

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

This is an implementation detail anyhow so instead of building for the future I'd rather build for compatibility right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Once this is being implemented as bundler plugins, its possible to also make this configurable, with appropriate defaults. For example, if a bundler is configured to output ESM, it might inject the more compact ESM snippet, falling back to the other one in case of CommonJS/UMD output.

text/0081-sourcemap-debugid.md Outdated Show resolved Hide resolved
text/0081-sourcemap-debugid.md Show resolved Hide resolved
text/0081-sourcemap-debugid.md Outdated Show resolved Hide resolved
text/0081-sourcemap-debugid.md Outdated Show resolved Hide resolved
text/0081-sourcemap-debugid.md Outdated Show resolved Hide resolved
@marandaneto
Copy link
Contributor

marandaneto commented Mar 27, 2023

Have we considered languages/frameworks that are transpiled to JS? Such as Flutter Web (Dart -> JS), you don't have much control over the transpilation and minification steps, nor you do have build hooks, not sure how the inject would work here.

@Swatinem
Copy link
Member Author

@marandaneto
I have not considered Dart / Flutter at all so far, and I do not know anything about it either. Can you give more details here? How does the compilation pipeline look like? Does Dart use SourceMaps? If so, how? Do developers usually combine that pipeline with other JS-native build tools/bundlers?

@marandaneto
Copy link
Contributor

marandaneto commented Mar 27, 2023

@marandaneto I have not considered Dart / Flutter at all so far, and I do not know anything about it either. Can you give more details here? How does the compilation pipeline look like? Does Dart use SourceMaps? If so, how? Do developers usually combine that pipeline with other JS-native build tools/bundlers?

Yeah Flutter web spits out source maps, and our symbolication works except for a few optimizations on the Dart side (as of now not a problem at all).
I guess it's possible to combine with other build tools/bundlers but it's not a common case at all, nor I have seen that, nor is documented, but maybe technically possible, I guess we don't need to support this at all at least as of now.
People just do flutter build web --source-maps and the magic happens.
Not sure which tooling flutter uses for source maps generation, would need to dig into the source code, here and here.

@the-spyke
Copy link

Could be also hash(minifiedContent + sourceMap)

We want to make processing / SourceMap-ing of JavaScript stack traces more reliable.
To achieve this, we want to uniquely identify a (minified / deployed) JavaScript file using a DebugId.
The same DebugId also uniquely identifies the corresponding SourceMap.
That way it should be possible to _reliably_ look up the SourceMap corresponding to
a JavaScript file, which is necessary to have reliable SourceMap processing.
@lforst
Copy link
Member

lforst commented Mar 31, 2023

Could be also hash(minifiedContent + sourceMap)

Don't know about this, since in some cases we don't have either minifiedContent or sourceMap available. (e.g. when injecting during bundler builds)

There are ways to make the debug id deterministic, but I don't think we can or should enforce the "origin of determinism".

@iambriccardo
Copy link
Member

iambriccardo commented Apr 3, 2023

Considering that our goal is to capture change as defined by any change in the non-minified source, the hash of the source map is our best bet. The only caveat I am thinking of is if there is the possibility that hash(sm1) != hash(sm2) if sc1 == sc2 where sm is the source map and sc is the non-minified source. An example of this is if the bundler omits/adds some information in the source map for some reason even if the original source stays the same.

@mitsuhiko
Copy link
Member

mitsuhiko commented Apr 6, 2023

Auxiliary options with source hashes: Original Spec from Edge

@Swatinem
Copy link
Member Author

Swatinem commented Apr 6, 2023

Do you know if the SourceHash proposal still takes suggestions? I would recommend to be able to explicitly state the hash algorithm, similar to https://github.com/dotnet/runtime/blob/main/docs/design/specs/PE-COFF.md#pdb-checksum-debug-directory-entry-type-19 to make it forward compatible to changing the hash algorithm.
In your example SourceBundle manifest, you have prefixed the digest with the algorithm as well. And that example snippet is missing the corresponding hash for the SourceMap entry.

@mitsuhiko
Copy link
Member

@Swatinem as the source hash proposal is already implemented in Chrome I'm not sure how many changes will happen there.

I moved the source hash proposal from my comment here: #85

@mitsuhiko
Copy link
Member

I think we generally want to merge this RFC with some minor edits, but we need to also document somewhere, what the changes to the artifact bundle are. I'm okay if this is not in the RFC as we already have it somewhere, but as it stands it's undocumented.

@mitsuhiko mitsuhiko merged commit ab0f755 into main Apr 13, 2023
@mitsuhiko mitsuhiko deleted the sourcemap-debugid branch April 13, 2023 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants