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(compile): disable source mapping of errors #8581

Merged
merged 1 commit into from Dec 1, 2020

Conversation

bartlomieju
Copy link
Member

This commit disables source mapping of errors
for standalone binaries. Since applying source
maps relies on using file fetcher infrastructure
it's not feasible to use it for standalone binaries
that are not supposed to use that infrastructure.

Closes #8578

This commit disables source mapping of errors
for standalone binaries. Since applying source
maps relies on using file fetcher infrastructure
it's not feasible to use it for standalone binaries
that are not supposed to use that infrastructure.
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

LGTM as tactical solution until #8577 and #4499 are delivered.

@bartlomieju bartlomieju merged commit f49d955 into denoland:master Dec 1, 2020
@bartlomieju bartlomieju deleted the fix_standalone_error branch December 1, 2020 22:33
@nayeemrmn
Copy link
Collaborator

We should still change the specifier to not use file URL hosts. The error site shows that we have liberally used file_url.to_file_path().unwrap() which it breaks. It should also be more similar to the other $deno$<subcommand>.js names we have. Easiest fix would be file:///$deno$bundle.js. I think the best would be file:///path/to/<exe>.$deno$bundle.js

@bartlomieju
Copy link
Member Author

@nayeemrmn that was already suggested by Kitson in #8578 (comment); I think we should go with some "magic" protocol instead of appending to current exe path. WDYT?

@nayeemrmn
Copy link
Collaborator

I suggested a file URL because it matches the virtual modules of other subcommands. But maybe it's unfortunate to use a file URL which isn't in the FS, if so we should change all of them.

@bartlomieju
Copy link
Member Author

I suggested a file URL because it matches the virtual modules of other subcommands. But maybe it's unfortunate to use a file URL which isn't in the FS, if so we should change all of them.

I guess deno:// scheme would make sense?

@nayeemrmn
Copy link
Collaborator

I guess deno:// scheme would make sense?

We use deno:cli/rt/... for internal bootstrap scripts. I think user code should always be something else.

@kitsonk
Copy link
Contributor

kitsonk commented Dec 2, 2020

I am of the opinion of vfs:// in advance of potentially supporting some sort of virtual file system.

I agree that deno:// should be reserved for internal stuff...

The problem with the other subcommands, especially test, that I ran across is that the module expects to be relative... There is an argument though that test should have absolute specifiers it imports anyways. That would be better IMO, I have always hated that it was a "lie".

@bartlomieju
Copy link
Member Author

bartlomieju commented Dec 2, 2020

I guess deno:// scheme would make sense?

We use deno:cli/rt/... for internal bootstrap scripts. I think user code should always be something else.

Right, we should distinct user code.

I am of the opinion of vfs:// in advance of potentially supporting some sort of virtual file system.

👍 agreed

The problem with the other subcommands, especially test, that I ran across is that the module expects to be relative... There is an argument though that test should have absolute specifiers it imports anyways. That would be better IMO, I have always hated that it was a "lie".

I think this stems from one of the first versions of deno test that tried to be too smart to cheat deno's TS compiler (shame on me 😅 )

@nayeemrmn
Copy link
Collaborator

Actually, the relatively located file URLs are essential for $deno$repl.ts, $deno$stdin.ts, $deno$eval.ts for useful dynamic import resolution...

@kitsonk
Copy link
Contributor

kitsonk commented Dec 2, 2020

@nayeemrmn true... we should "fix" it for deno test though. The other option is to modify the module graph to have a different resolution base for these virtual files, instead of "faking" them. They still need to be inserted into the file fetcher, but as long as we agree to the semantics of how to resolve relative dynamic imports (relative to cwd), that should be straight forward for the module graph to handle.

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.

deno compile stack trace printing causes panics
3 participants