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

doc: file url in stack traces contains file:// #39787

Closed
1 task
hyrious opened this issue Aug 17, 2021 · 12 comments
Closed
1 task

doc: file url in stack traces contains file:// #39787

hyrious opened this issue Aug 17, 2021 · 12 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@hyrious
Copy link

hyrious commented Aug 17, 2021

📗 API Reference Docs Problem

  • Version: v16.6.2

  • Platform: Darwin user.local 20.6.0 Darwin Kernel Version 20.6.0: Wed Jun 23 00:26:31 PDT 2021; root:xnu-7195.141.2~5/RELEASE_X86_64 x86_64

  • Subsystem: -

Location

Affected URL(s):

Description

The doc says

The location information will be one of:

  • native, if the frame represents a call internal to V8 (as in [].forEach).
  • plain-filename.js:line:column, if the frame represents a call internal to Node.js.
  • /absolute/path/to/file.js:line:column, if the frame represents a call in a user program, or its dependencies.

However the error stack trace actually contains file:// in es module files.
I'm not sure if it occurs in other situations.

Consider this input:

throw new Error("hello, world!");

Save and run node a.js:

/Users/hyrious/test-trace/a.js:1
throw new Error("hello, world!");
^

Error: hello, world!
    at Object.<anonymous> (/Users/hyrious/test-trace/a.js:1:7)

Save and run node a.mjs:

file:///Users/hyrious/test-trace/a.mjs:1
throw new Error("hello, world!");
      ^

Error: hello, world!
    at file:///Users/hyrious/test-trace/a.mjs:1:7

What I have to mention here is sindresorhus/clean-stack#27, using normal path would be better in many terminals. Please take that into consideration too.

My suggestion is node.js always returning normal path in error stack trace.


  • I would like to work on this issue and
    submit a pull request.
@hyrious hyrious added the doc Issues and PRs related to the documentations. label Aug 17, 2021
@aduh95 aduh95 added the good first issue Issues that are suitable for first-time contributors. label Aug 17, 2021
@Phantomghost1

This comment has been minimized.

@prabhatmishra33
Copy link

Hello! Is this open for contribution ? I would like to take up this.

@Qard
Copy link
Member

Qard commented Dec 4, 2021

@nodejs/loaders I had someone ask about making the stack trace file paths consistent rather than documenting the differences. I expect there may be some correctness concerns about fully-qualified file URLs though so could use clarification here on what the "correct" fix would be, or if one can be made. I know historically Node.js has leaned a bit towards changes to error output being a breaking change.

@JakobJingleheimer
Copy link
Contributor

I think that file:// is soon to be important: when #36328 lands, files will no-longer always be local, so if the prefix is stripped, the user will lose valuable information.

@Qard
Copy link
Member

Qard commented Dec 4, 2021

True. So maybe the file:// prefix should be added to cjs script names to match?

@JakobJingleheimer
Copy link
Contributor

Yeah, I think consistency would be better even though CJS is limited to always being a local file. I'm not sure whether it's worth the breaking change (if it were just me, I would want the consistency).

But, @hyrious mentioned it's better for many terminals—why? Perhaps adding the file:// prefix for the consistency could be detrimental.

@anonrig
Copy link
Member

anonrig commented Dec 4, 2021

I'd be happy to take a look at it if we're ok with a breaking change in CJS.

@hyrious
Copy link
Author

hyrious commented Dec 4, 2021

Not that detrimental. When I asked this problem, terminals (iTerm2, etc.) ware not able to recognize file:/// to mark them as clickable (to open file in editor quickly). This is maybe trivial to implement, but is still a new rule to let many terminals agree with, some of them may not update for years.

It's ok to see file:// everywhere in Node.js context, if you think this is correct.

@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Dec 5, 2021

I just checked iTerm2, and a file:// prefix does NOT interfere with URL recognition nor with opening the linked file:

2021-12-05_20-06_iTerm2-reconising-file-protocol-urls

I command-clicked that link, and it opened that test file in the default text editor.

Note that iTerm2 does not linkify URLs to files that do not actually exist.

I also confirmed iTerm2 does linkify https:// URLs too, so both cases are already supported.

We can't check all terminal apps, but I think iTerm2 is the big one, so I think we should do as @Qard suggested and backport the file:// to CJS.

@ljharb
Copy link
Member

ljharb commented Dec 5, 2021

Why should CJS have file://? CJS specifiers are file paths, not URLs.

The most important consistency is that errors are consistent with the relevant module system. The two module systems have deliberate differences, and some of the inconsistencies between them are sadly intentional and unlikely to ever go away.

@anonrig
Copy link
Member

anonrig commented Dec 5, 2021

Since it's required to have file:// in MJS due to the multiple protocol support; the problem stays within the consistency of CJS and the MJS. Even though it is a breaking change, the change is already handled by iTerm as it seems. I can open a pull request and introduce the change, but just wanted to align on it first.

@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Dec 5, 2021

Quick update from a slack conversation with @Qard, @ljharb, @anonrig, and me:

CJS cannot handle a file:// protocol prefix, only paths (rather than a URL), so adding it to the error output would be detrimental because it could not be used as input, yet would suggest it can.

So as-is, the output should not be updated (without a change to CJS to actually handle that as input, which is a separate debate).

So I think that leaves us with 2 options:

  • Do nothing
  • Document the difference

thebergamo added a commit to thebergamo/node that referenced this issue Dec 13, 2021
This change highlihts in the docs difference between stack traces for CJS modules and ES Modules.

Fixes: nodejs#39787
thebergamo added a commit to thebergamo/node that referenced this issue Dec 13, 2021
This change highlights in the docs difference between stack traces for
CommonJS modules and ES Modules.

Fixes: nodejs#39787
thebergamo added a commit to thebergamo/node that referenced this issue Dec 13, 2021
This change highlights in the docs difference between stack traces for
CommonJS modules and ES Modules.

Fixes: nodejs#39787
targos pushed a commit that referenced this issue Jan 14, 2022
This change highlights in the docs difference between stack traces for
CommonJS modules and ES Modules.

Fixes: #39787

PR-URL: #41157
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
This change highlights in the docs difference between stack traces for
CommonJS modules and ES Modules.

Fixes: #39787

PR-URL: #41157
Reviewed-By: James M Snell <jasnell@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
This change highlights in the docs difference between stack traces for
CommonJS modules and ES Modules.

Fixes: nodejs#39787

PR-URL: nodejs#41157
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
This change highlights in the docs difference between stack traces for
CommonJS modules and ES Modules.

Fixes: #39787

PR-URL: #41157
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants