errors: do not call resolve on URLs with schemes by bcoe · Pull Request #35903 · nodejs/node · GitHub

Based on advice from webpack project, this PR stops trying to resolve
sources with a webpack:// scheme (or other schemes, as they represent
absolute URLs). Source content is now loaded from the sourcesContent
lookup, if it is populated (allowing for non file:// schemes).

stack-trace:

webpack:///./webpack.js:14
  throw new Error('oh no!')
        ^

Error: oh no!
    at o (/Users/bencoe/oss/node-1/test/fixtures/source-map/webpack.js:1:970)
        -> webpack:///./webpack.js:14:9
    at n (/Users/bencoe/oss/node-1/test/fixtures/source-map/webpack.js:1:952)
        -> webpack:///./webpack.js:10:3

This work is in pursuit of #35325

Refs webpack/webpack#9601

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

CC: @sokra, @nodejs/tooling

I do not think we should start including explicit special cases in node core for third party libraries like this. We should be solving this in a more generic way if there is not already some generic API/interface that suffices for webpack.

@mscdex take another look, I abstracted the logic so that it should apply to any paths with a scheme; I think the resolve logic would have potentially been broken for file:// paths prior to this.

@mscdex based on @sokra's reading of the Source Map v3 language, I cleaned up the logic further. If we have scheme:// we shouldn't need to ever resolve.

Based on advice from webpack project, this PR stops trying to resolve sources with a webpack:// scheme (or other schemes, as they represent absolute URLs). Source content is now loaded from the sourcesContent lookup, if it is populated (allowing for non file:// schemes). Refs: nodejs#35325

CI: https://ci.nodejs.org/job/node-test-pull-request/34004/

Co-authored-by: devsnek <me@gus.host>

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

CI: https://ci.nodejs.org/job/node-test-pull-request/34013/ ✅

👋 if no one objects, I'd like to land this towards the end of the day, since there are a few additional fixes I'd like to make to source-map handling which build on this.

lgtm

Landed in 26fcdb6

We were incorrectly trying to run path.resolve on absolute sources URLs. This was breaking webpack:// URLs in stack trace output. Refs: #35325 PR-URL: #35903 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

We were incorrectly trying to run path.resolve on absolute sources URLs. This was breaking webpack:// URLs in stack trace output. Refs: nodejs#35325 PR-URL: nodejs#35903 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

We were incorrectly trying to run path.resolve on absolute sources URLs. This was breaking webpack:// URLs in stack trace output. Refs: #35325 PR-URL: #35903 Backport-PR-URL: #37717 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

bcoe requested review from boneskull and devsnek October 31, 2020 17:19

sokra suggested changes Oct 31, 2020

View reviewed changes

Comment thread lib/internal/source_map/source_map_cache.js Outdated

mscdex previously requested changes Oct 31, 2020

View reviewed changes

bcoe requested a review from mscdex October 31, 2020 20:58

bcoe changed the title process: handle webpack:// scheme for source-maps process: handle alternative schemes in source-maps Oct 31, 2020

bcoe changed the title process: handle alternative schemes in source-maps errors: handle alternative schemes in source-maps Oct 31, 2020

bcoe changed the title errors: handle alternative schemes in source-maps errors: do not call resolve on URLs with schemes Oct 31, 2020

bcoe force-pushed the webpack-fix branch from d4ef530 to 44f20fc Compare October 31, 2020 23:08

devsnek reviewed Nov 1, 2020

View reviewed changes

Comment thread lib/internal/source_map/source_map_cache.js Outdated

Update lib/internal/source_map/source_map_cache.js

dd0949f

Co-authored-by: devsnek <me@gus.host>

sokra suggested changes Nov 1, 2020

View reviewed changes

Comment thread lib/internal/source_map/prepare_stack_trace.js Outdated

Comment thread lib/internal/source_map/source_map_cache.js Outdated

Comment thread lib/internal/source_map/source_map_cache.js Outdated

Dragiyski mentioned this pull request Nov 1, 2020

lib: fix inspector links for stack traces #35725

Closed

3 tasks

aduh95 reviewed Nov 1, 2020

View reviewed changes

Comment thread lib/internal/source_map/prepare_stack_trace.js Outdated

Comment thread lib/internal/source_map/prepare_stack_trace.js Outdated

Comment thread lib/internal/source_map/prepare_stack_trace.js Outdated

Comment thread lib/internal/source_map/source_map_cache.js Outdated

Benjamin E. Coe and others added 5 commits November 1, 2020 10:38

Update lib/internal/source_map/prepare_stack_trace.js

3e78647

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Update lib/internal/source_map/prepare_stack_trace.js

596cf2a

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Update lib/internal/source_map/source_map_cache.js

8933b5c

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Update lib/internal/source_map/prepare_stack_trace.js

6ace772

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

errors: use URL for resolution

21124ef

bcoe requested review from aduh95 and devsnek November 1, 2020 16:17

aduh95 reviewed Nov 1, 2020

View reviewed changes

Comment thread lib/internal/source_map/prepare_stack_trace.js Outdated

Comment thread lib/internal/source_map/prepare_stack_trace.js Outdated

aduh95 reviewed Nov 1, 2020

View reviewed changes

Comment thread lib/internal/source_map/prepare_stack_trace.js Outdated

chore: address code review

3c1a7ca

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

bcoe added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 2, 2020

bcoe requested a review from aduh95 November 2, 2020 14:40

Trott approved these changes Nov 2, 2020

View reviewed changes

aduh95 approved these changes Nov 2, 2020

View reviewed changes

mcollina approved these changes Nov 2, 2020

View reviewed changes

bcoe closed this Nov 3, 2020

bcoe deleted the webpack-fix branch November 3, 2020 00:31

targos mentioned this pull request Nov 3, 2020

v15.1.0 release proposal #35948

Merged

BethGriggs added the backport-requested-v14.x label Dec 7, 2020

bcoe mentioned this pull request Mar 10, 2021

[v14.x backport] errors: print original exception context #37700

Closed

bcoe mentioned this pull request Mar 11, 2021

[v14.x backport] errors: do not call resolve on URLs with schemes #37717

Closed

targos added backport-open-v14.x and removed backport-requested-v14.x labels Apr 25, 2021

targos added backported-to-v14.x errors Issues and PRs related to JavaScript errors originated in Node.js core. and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. backport-open-v14.x labels Apr 25, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

errors: do not call resolve on URLs with schemes by bcoe · Pull Request #35903 · nodejs/node · GitHub

Uh oh!

Uh oh!

errors: do not call resolve on URLs with schemes - #35903

errors: do not call resolve on URLs with schemes#35903bcoe wants to merge 10 commits intonodejs:masternodejs/node:masterfrom bcoe:webpack-fixbcoe/node-1:webpack-fixCopy head branch name to clipboard

Conversation

bcoe commented Oct 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

mscdex left a comment

Choose a reason for hiding this comment

Uh oh!

bcoe commented Oct 31, 2020

Uh oh!

bcoe commented Oct 31, 2020

Uh oh!

nodejs-github-bot commented Oct 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 2, 2020 • edited by bcoe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcoe commented Nov 2, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

bcoe commented Nov 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

errors: do not call resolve on URLs with schemes#35903
bcoe wants to merge 10 commits into
nodejs:masterfrom
bcoe:webpack-fix

bcoe commented Oct 31, 2020 •
edited
Loading

nodejs-github-bot commented Nov 2, 2020 •
edited by bcoe
Loading