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.

Sign up for GitHub

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

Jump to bottom

[WIP] feat: Add native SourceMap support #819

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

[WIP] feat: Add native SourceMap support #819

wants to merge 31 commits into from
+442 −50

Conversation

bartlomieju
Copy link
Member

bartlomieju
bartlomieju commented Jul 11, 2024
View reviewed changes
core/ops_builtin_v8.rs Outdated Show resolved Hide resolved
core/runtime/jsruntime.rs Outdated Show resolved Hide resolved
core/source_map.rs Outdated Show resolved Hide resolved
bartlomieju
bartlomieju commented Jul 11, 2024
View reviewed changes
core/source_map.rs Outdated Show resolved Hide resolved
bartlomieju
bartlomieju commented Jul 11, 2024
View reviewed changes
core/source_map.rs Outdated Show resolved Hide resolved
bartlomieju
bartlomieju commented Jul 11, 2024
View reviewed changes
core/source_map.rs Outdated Show resolved Hide resolved
@bartlomieju bartlomieju mentioned this pull request Jul 12, 2024
Merged
bartlomieju added a commit that referenced this pull request Jul 12, 2024
@bartlomieju
refactor: Simplify SourceMapGetter and SourceMapper (#821)
b5218a5 
Splitting out a simplification from
#819.
@bartlomieju bartlomieju mentioned this pull request Jul 13, 2024
Merged
bartlomieju added a commit that referenced this pull request Jul 15, 2024
@bartlomieju
feat: Depcrate SourceMapGetter, move methods to ModuleLoader (#823)
b4d48a1 
This commit deprecates `SourceMapGetter` trait,
`JsRuntime::source_map_getter`
option and updates `ModuleLoader` trait to have the same methods as
`SourceMapGetter`.

In real usage it's always implementor of `ModuleLoader` that also
implements
`SourceMapGetter`. It makes little sense to keep these two trait
separate.

This will open up doors for native source map support that is being
developed in #819.

`SourceMapGetter` is scheduled to be removed in 0.297.0.
@bartlomieju
Copy link
Member Author

sourceMapURL being a "bare" url (like bar.js.map) is currently not handled properly by any loader.

I think I'm gonna change ModuleLoader::get_source_map to be two separate functions:

  • ModuleLoader::get_source_map_for_file() - last ditch effort to get a source map for particular file_name - this is mechanism deno CLI uses right now, by stripping out sourceMapURL before passing source code to V8
  • ModuleLoader::load_source_map(file_name: &str) - given a resolved URL synchronously return a source map file. Given that most often the URL in sourceMapURL is actually not a relative URL (eg. bar.js.map) a bit of logic is required in SourceMapper to append ./ to that value and trying to resolve it again. I'm still thinking how to handle it best.

@marvinhagemeister
Copy link
Contributor

sourceMapURL being a "bare" url (like bar.js.map) is currently not handled properly by any loader.

I think we have that in Preact. We've gotten reports for a while that the source maps don't load (outside of Deno) and this could explain why. I didn't know that the paths were expected to begin with ./ 👀

@bartlomieju
Copy link
Member Author

sourceMapURL being a "bare" url (like bar.js.map) is currently not handled properly by any loader.

I think we have that in Preact. We've gotten reports for a while that the source maps don't load (outside of Deno) and this could explain why. I didn't know that the paths were expected to begin with ./ 👀

I don't think it's expected to being with ./ - most tools put ULR like bar.js.map there. What I meant be my comment is that our loaders need to be updated to handle such "url". Basically if the parsing fails, we should prepend ./ and try to resolve it against the file that url is in.

bartlomieju
bartlomieju commented Jul 17, 2024
View reviewed changes
core/error.rs
Comment on lines -361 to -547
if !file_name.trim_start_matches('[').starts_with("ext:") {
source_line = source_mapper.get_source_line(file_name, line_number);
source_line_frame_index = Some(i);
break;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Verify this in a separate PR. We support source maps for ext: modules so most likely this is not needed

bartlomieju
bartlomieju commented Jul 17, 2024
View reviewed changes
core/source_map.rs Outdated Show resolved Hide resolved
bartlomieju
bartlomieju commented Jul 17, 2024
View reviewed changes
core/source_map.rs Outdated Show resolved Hide resolved
bartlomieju
bartlomieju commented Jul 17, 2024
View reviewed changes
core/source_map.rs Outdated
Comment on lines 133 to 136
let source_map = if let Some(b64) =
source_map_string.strip_prefix("data:application/json;base64,")
{
let decoded_b64 = BASE64_STANDARD.decode(b64).ok()?;
Copy link
Member Author

Choose a reason for hiding this comment

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

This code is duplicated in CLI, maybe we can have a common helper shared between the two?

@bartlomieju bartlomieju mentioned this pull request Jul 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bartlomieju @marvinhagemeister @devsnek