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

new vendor feature: Stack traces point to original https: urls #20083

Open
marvinhagemeister opened this issue Aug 7, 2023 · 7 comments
Open
Labels
needs discussion this topic needs further discussion to determine what action to take

Comments

@marvinhagemeister
Copy link
Contributor

With the new vendor feature added in #20065 and #15633 dependencies are resolved locally which is fantastic. I noticed that when an error is thrown the stack trace still points to the original https: urls instead of the local files. This means that I'm transfered to the website in the browser when I click on the url instead of it opening the local file.

Screenshot 2023-08-07 at 11 25 49

Steps to reproduce

  1. Clone https://github.com/marvinhagemeister/deno-vendor-stack
  2. Run deno run -A main.ts
@marvinhagemeister marvinhagemeister changed the title Unstable vendor: Stack traces point to original https: urls new vendor feature: Stack traces point to original https: urls Aug 7, 2023
@nayeemrmn
Copy link
Collaborator

I think it's not great if vendoring affects runtime behaviour, that was one of the motivations for #15633. Isn't there a way for vscode_deno to intercept link clicks and offer to open the relevant cache file?

@marvinhagemeister
Copy link
Contributor Author

Ok, here is a use case where showing the original URL is the wrong thing to do. Adding a function at the end of a dependency like a new export. In this case lets add a new export to preact:

/* esm.sh - preact@10.16.0 */
export * from "https://esm.sh/stable/preact@10.16.0/denonext/preact.mjs";

export function thrower() {
  throw new Error("fail");
}

So now I import thrower in my code and call it. The stack trace that I get looks like this:

error: Uncaught Error: fail
  throw new Error("fail");
        ^
    at thrower (https://esm.sh/preact@10.16.0:5:9)
    at file:///project/main.ts:4:1

Now I go to the https://esm.sh/preact@10.16.0 to check what is happening at line 5, column 3. This is what the content returned by the URL looks like

/* esm.sh - preact@10.16.0 */
export * from "https://esm.sh/stable/preact@10.16.0/es2022/preact.mjs";

But wait a second, this file only has two lines? The stack trace location shows that there'd be at least 5 lines in there. So at this point the stack trace is essentially lying to the user.

@marvinhagemeister marvinhagemeister added the bug Something isn't working label Aug 7, 2023
@nayeemrmn
Copy link
Collaborator

Ok, here is a use case where showing the original URL is the wrong thing to do. Adding a function at the end of a dependency like a new export.

By that rationale, even if we changed the stack frames, the user's own import statements would still be lying to them right? To reconcile that there is no choice but to think of the vendor directory as sort of a network proxy.

This also would lead to a worse experience for lib authors with respect to bug reports, online helpers, etc..

So if we can solve the UX problem in the VSCode extension that would be ideal...

@marvinhagemeister
Copy link
Contributor Author

marvinhagemeister commented Aug 7, 2023

I don't know. I was more expecting the feature to work like a virtual alias. Something that only matters during the resolution phase. I'd be totally fine with the import statements not lining up. That to me that would be the expected behavior.

I'm not sure if we should only cater to vscode users, even if it's the most popular editor among web devs.

@marvinhagemeister
Copy link
Contributor Author

marvinhagemeister commented Aug 7, 2023

FWIW Go shows the file path instead of the URL:

goroutine 6 [chan receive]:
go.elastic.co/apm.(*Tracer).loop.func2(0xc000042420, 0xc0000423c0, 0x1551980, 0xc0004da100, 0xc000074300, 0xc000001080, 0xc00000e018)
    /app/vendor/go.elastic.co/apm/tracer.go:803 +0x21a
created by go.elastic.co/apm.(*Tracer).loop
    /app/vendor/go.elastic.co/apm/tracer.go:800 +0x36e

@lucacasonato
Copy link
Member

I expect the current behavior. I do think we should augment the stack traces with the vendor path. Eg at https://example.com/main.js:3:1 (via file:///vendor/example.com/main.js:5:3)

import.meta.url should point to the HTTPS specifier

@lucacasonato lucacasonato added needs discussion this topic needs further discussion to determine what action to take and removed bug Something isn't working labels Aug 14, 2023
@marvinhagemeister
Copy link
Contributor Author

Showing both works too. Reminds me of how some CLI's present source mapped stack traces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion this topic needs further discussion to determine what action to take
Projects
None yet
Development

No branches or pull requests

3 participants