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

bcoe · 2020-10-31T17:19:50Z

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

lib/internal/source_map/source_map_cache.js

mscdex

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.

bcoe · 2020-10-31T20:59:48Z

@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.

bcoe · 2020-10-31T21:29:08Z

@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

nodejs-github-bot · 2020-10-31T23:17:59Z

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

lib/internal/source_map/source_map_cache.js

Co-authored-by: devsnek <[email protected]>

lib/internal/source_map/prepare_stack_trace.js

lib/internal/source_map/source_map_cache.js

lib/internal/source_map/prepare_stack_trace.js

lib/internal/source_map/source_map_cache.js

Co-authored-by: Antoine du Hamel <[email protected]>

lib/internal/source_map/prepare_stack_trace.js

Co-authored-by: Antoine du Hamel <[email protected]>

nodejs-github-bot · 2020-11-02T02:52:39Z

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

bcoe · 2020-11-02T18:30:10Z

👋 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.

mcollina

lgtm

bcoe · 2020-11-03T00:31:54Z

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

We were incorrectly trying to run path.resolve on absolute sources URLs. This was breaking webpack:// URLs in stack trace output. Refs: nodejs#35325 Backport-PR-URL: nodejs#37717 PR-URL: nodejs#35903 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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

sokra suggested changes Oct 31, 2020
View reviewed changes

lib/internal/source_map/source_map_cache.js Outdated Show resolvedHide resolved

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-mapsOct 31, 2020

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

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

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

devsnek reviewed Nov 1, 2020
View reviewed changes

lib/internal/source_map/source_map_cache.js Outdated Show resolvedHide resolved

Update lib/internal/source_map/source_map_cache.js
dd0949f
Co-authored-by: devsnek <[email protected]>

sokra suggested changes Nov 1, 2020
View reviewed changes

lib/internal/source_map/prepare_stack_trace.js Outdated Show resolvedHide resolved
lib/internal/source_map/source_map_cache.js Outdated Show resolvedHide resolved
lib/internal/source_map/source_map_cache.js Outdated Show resolvedHide resolved

Dragiyski mentioned this pull request Nov 1, 2020
lib: fix inspector links for stack traces #35725
Open
3 tasks

aduh95 reviewed Nov 1, 2020
View reviewed changes

Benjamin E. Coeand 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 <[email protected]>

Update lib/internal/source_map/prepare_stack_trace.js
596cf2a
Co-authored-by: Antoine du Hamel <[email protected]>

Update lib/internal/source_map/source_map_cache.js
8933b5c
Co-authored-by: Antoine du Hamel <[email protected]>

Update lib/internal/source_map/prepare_stack_trace.js
6ace772
Co-authored-by: Antoine du Hamel <[email protected]>

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

lib/internal/source_map/prepare_stack_trace.js Outdated Show resolvedHide resolved
lib/internal/source_map/prepare_stack_trace.js Outdated Show resolvedHide resolved

aduh95 reviewed Nov 1, 2020
View reviewed changes

lib/internal/source_map/prepare_stack_trace.js Outdated Show resolvedHide resolved

chore: address code review
3c1a7ca
Co-authored-by: Antoine du Hamel <[email protected]>

bcoe added the author ready PRs that have at least one approval, no pending requests for changes, 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 pending requests for changes, 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

Uh oh!

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

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

Uh oh!

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

bcoe commented Oct 31, 2020•
edited
Loading

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