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

fix(op_crates/web): Use "deno:" URLs for internal script specifiers #7383

Merged
merged 15 commits into from
Sep 9, 2020

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Sep 7, 2020

Fixes deno.exe eval "new Event()" panic. Described in #7369 (comment).

Internal script specifiers use URLs in the form of deno:<repo-relative path to file>. That lets Deno developers conveniently Ctrl+click the paths (checked in VSCode).

image

Additionally, having a clear prefix lets us restore dimming for internal stack frames: #4201.

The panic is fixed by ignoring deno: URLs when source mapping.

@nayeemrmn nayeemrmn marked this pull request as ready for review September 7, 2020 16:55
@nayeemrmn

This comment has been minimized.

@nayeemrmn nayeemrmn changed the title fix(op_crates/web): Use file URLs as script specifiers fix(op_crates/web): Use relative paths as script specifiers Sep 7, 2020
@nayeemrmn
Copy link
Collaborator Author

cc @bartlomieju

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Thanks for the patch - looks good.

I wonder if you could experiment with moving the test into the deno_web crate? Since this is a bug in deno_web, ideally the test would be there too.

This is experimental because we haven't yet developed good patterns for how to do tests in op crates. However this is a pretty simple situation - so it will be an instructive experiment. Consider not using itest.

@bartlomieju
Copy link
Member

@nayeemrmn thanks for this catch! The code looks good, I only wonder if we should show op_crates/web/<filename> instead of web/<filename>. @ry WDYT?

@nayeemrmn
Copy link
Collaborator Author

I wonder if you could experiment with moving the test into the deno_web crate? Since this is a bug in deno_web, ideally the test would be there too.

I'll look into adding a local test for relative script names. The panic is occurring in CLI, I think the itest should stay to test a component of op_crates integration.

I only wonder if we should show op_crates/web/<filename> instead of web/<filename>

I agree, also we should show cli/rt/... instead of rt/... so the root is consistent.

@ry
Copy link
Member

ry commented Sep 8, 2020

I only wonder if we should show op_crates/web/ instead of web/. @ry WDYT?

I guess that would be better...

It would be cool to eventually use some sort of absolute URL (maybe crate://deno@1.3.3/op_crates/web/01_event.js)

@nayeemrmn
Copy link
Collaborator Author

nayeemrmn commented Sep 8, 2020

It would be cool to eventually use some sort of absolute URL (maybe crate://deno@1.3.3/op_crates/web/01_event.js)

I was actually just experimenting with deno:op_crates/web/.... This is likely the most appropriate URL format since it's just protocol + pathname. It also works with Ctrl+click in integrated terminals when developing Deno:
image

Some kind of prefix is necessary to restore #4201, and we could do cool things like showing the source line for the first non-deno stack frame.

@nayeemrmn nayeemrmn changed the title fix(op_crates/web): Use relative paths as script specifiers fix(op_crates/web): Use "deno:" URLs for internal script specifiers Sep 8, 2020
@nayeemrmn
Copy link
Collaborator Author

@ry Please review.

op_crates/web/lib.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

Nitpick: why deno:op_crates/web instead of deno://op_crates/web?

@nayeemrmn
Copy link
Collaborator Author

Nitpick: why deno:op_crates/web instead of deno://op_crates/web?

@bartlomieju deno://op_crates/web makes op_crates the host, quite arbitrarily.

A bit better would be deno:///op_crates/web but that makes it an absolute path which would never be correctly meaningful. It's not Ctrl+clickable, for example.

@@ -0,0 +1,3 @@
[WILDCARD]error: Uncaught TypeError: Event requires at least 1 argument, but only 0 present[WILDCARD]
at new Event (deno:op_crates/web/[WILDCARD])
Copy link
Member

Choose a reason for hiding this comment

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

I think this seems pretty reasonable and clean.

@bartlomieju please land if you're okay with it this too.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree, @nayeemrmn's right about the host part.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @nayeemrmn

@bartlomieju bartlomieju merged commit b17a5fb into denoland:master Sep 9, 2020
@nayeemrmn nayeemrmn deleted the op-crates-web-errors branch September 9, 2020 22:18
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.

None yet

3 participants