FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v14.x backport] errors: do not call resolve on URLs with schemes by bcoe · Pull Request #37717 · nodejs/node · GitHub

/ node Public

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

Closed
bcoe wants to merge 1 commit into
nodejs:v14.x-stagingfrom
bcoe:backport-webpack-fix
ConversationCommits (1)ChecksFiles changed
Closed

[v14.x backport] errors: do not call resolve on URLs with schemes#37717
bcoe wants to merge 1 commit into
nodejs:v14.x-stagingfrom
bcoe:backport-webpack-fix

Conversation

bcoe commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot added needs-ci PRs that need a full CI run. v14.x labels Mar 11, 2021
bcoe changed the title errors: do not call resolve on URLs with schemes [v14.x backport] errors: do not call resolve on URLs with schemes Mar 11, 2021
bcoe requested review from Trott, aduh95 and ruyadorno March 11, 2021 19:30
bcoe force-pushed the backport-webpack-fix branch from 8cae83d to 1cde0f9 Compare March 11, 2021 19:31

bcoe commented Mar 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This is part of a release train of backport PRs:

#33491 ✅
#35903 🚆
#35915
#37252
#37362

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Mar 12, 2021 •
edited by bcoe
Loading

Copy link
Copy Markdown
Collaborator

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

bcoe force-pushed the backport-webpack-fix branch from 35bd63e to 1cde0f9 Compare March 15, 2021 18:26
bcoe mentioned this pull request Mar 15, 2021
Closed
bcoe force-pushed the backport-webpack-fix branch from 1cde0f9 to 2197404 Compare March 16, 2021 00:47
bcoe force-pushed the backport-webpack-fix branch from 2197404 to f91f140 Compare March 16, 2021 00:59

bcoe commented Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

@aduh95 any idea where to go next with the test-asan issue. When I run locally --enable-asan fails on completely different tests (I'm guessing related to running on OSX, not this same failure we're seeing).

aduh95 commented Mar 16, 2021

Copy link
Copy Markdown
Contributor

@aduh95 any idea where to go next with the test-asan issue. When I run locally --enable-asan fails on completely different tests (I'm guessing related to running on OSX, not this same failure we're seeing).

Sorry I can't explain the failure either, it's really weird.

ruyadorno commented Mar 16, 2021

Copy link
Copy Markdown
Member

@bcoe is this same target failing the same way if you try running them in v14.x-staging?

bcoe commented Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

@ruyadorno v14.x-staging seems fine, so something about this PR is breaking the test. The weird thing is the test isn't failing on test-asan on the main branch either (which includes this change).

We might need to try to reproduce in a container locally.

bcoe force-pushed the backport-webpack-fix branch 2 times, most recently from 5c95073 to 0e1a9b5 Compare March 24, 2021 04:23
bcoe mentioned this pull request Mar 24, 2021
Closed
13 tasks
bcoe force-pushed the backport-webpack-fix branch from e59434d to e4ad78b Compare April 4, 2021 01:00

bcoe commented Apr 5, 2021

Copy link
Copy Markdown
Contributor Author

@targos @richardlau I've been unable to reproduce the test-asan issues on my own linux machine, and they seem to be happening in tests unrelated to this PR.

I noticed in #38001 we also landed a PR to 14.x-staging with test-asan failures, is it alright if I do likewise with this PR? (perhaps with a tracking issue to dig into test-asan issues on 14.x-staging).

nodejs-github-bot commented Apr 5, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 6, 2021

Copy link
Copy Markdown
Collaborator

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

errors: do not call resolve on URLs with schemes
753d783 
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>
targos force-pushed the backport-webpack-fix branch from e4ad78b to 753d783 Compare April 24, 2021 12:53

nodejs-github-bot commented Apr 24, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 25, 2021

Copy link
Copy Markdown
Collaborator

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

targos pushed a commit that referenced this pull request Apr 25, 2021
errors: do not call resolve on URLs with schemes
b01c496 
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>

targos commented Apr 25, 2021

Copy link
Copy Markdown
Member

Landed in b01c496

targos closed this Apr 25, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
aduh95 aduh95 approved these changes
ruyadorno Awaiting requested review from ruyadorno

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL