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

lib: don't parse windows drive letters as schemes by PaperStrike · Pull Request #50580 · nodejs/node · GitHub

/ node Public

lib: don't parse windows drive letters as schemes - #50580

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
PaperStrike:abs-win-path-sources
Dec 5, 2023
Merged

lib: don't parse windows drive letters as schemes#50580
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
PaperStrike:abs-win-path-sources

Conversation

Copy link
Copy Markdown
Contributor

The windows drive letters are incorrectly parsed as schemes, which pollutes the source map cache with malformed file paths.

Fixes: #50523

We were incorrectly parsing windows drive letters as schemes. This was
polluting the source map cache with malformed file paths.

Fixes: nodejs#50523
nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 6, 2023

Copy link
Copy Markdown
Contributor Author

@bcoe @cspotcode Any thoughts? Thanks in advance.

bcoe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks like a reasonable fix to me. I'm not a maintainer but giving my 👍

Copy link
Copy Markdown
Contributor Author

Hello, is anyone paying attention to this small fix? I am expecting this to be merged and backported to 18.x as soon as possible. Thank you guys.

Pinging some members with source-map-cache review experience. Hope this doesn't bothers much.
@anonrig @legendecas @joyeecheung

Comment thread test/parallel/test-source-map-enable.js Outdated

Copy link
Copy Markdown
Member

This looks like a trivial fix but I think it would be better to raise the issue to https://github.com/tc39/source-map-rfc as well.

legendecas added the source maps Issues and PRs related to source map support. label Nov 24, 2023

legendecas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with the comment change

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 3, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 3, 2023

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Contributor Author

The failure doesn't seem related to this PR. sequential.test-watch-mode-inspect was marked flaky.

Copy link
Copy Markdown
Collaborator

legendecas added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 5, 2023

Copy link
Copy Markdown
Member

Thank you for your contribution!

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 5, 2023

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/50580
✔  Done loading data for nodejs/node/pull/50580
----------------------------------- PR info ------------------------------------
Title      lib: don't parse windows drive letters as schemes (#50580)
Author     华  (@PaperStrike, first-time contributor)
Branch     PaperStrike:abs-win-path-sources -> nodejs:main
Labels     author ready, needs-ci, source maps
Commits    3
 - lib: don't parse windows drive letters as schemes
 - lib: typo fixes
 - lib: update sourcesToAbsolute comment
Committers 1
 - 华 
PR-URL: https://github.com/nodejs/node/pull/50580
Fixes: https://github.com/nodejs/node/issues/50523
Reviewed-By: Chengzhong Wu 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50580
Fixes: https://github.com/nodejs/node/issues/50523
Reviewed-By: Chengzhong Wu 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 06 Nov 2023 15:38:17 GMT
   ✔  Approvals: 1
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/50580#pullrequestreview-1764032021
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-12-05T06:11:16Z: https://ci.nodejs.org/job/node-test-pull-request/56095/
- Querying data for job/node-test-pull-request/56095/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 50580
From https://github.com/nodejs/node
 * branch                  refs/pull/50580/merge -> FETCH_HEAD
✔  Fetched commits as 9def0a9f94f0..f41d75815fa4
--------------------------------------------------------------------------------
Auto-merging lib/internal/source_map/source_map_cache.js
[main 6a092ac015] lib: don't parse windows drive letters as schemes
 Author: 华 
 Date: Mon Nov 6 12:22:41 2023 +0000
 4 files changed, 49 insertions(+)
 create mode 100644 test/fixtures/source-map/ts-node-win32.js
 create mode 100644 test/fixtures/source-map/ts-node.ts
[main 0bbf0ed739] lib: typo fixes
 Author: 华 
 Date: Sat Nov 25 10:40:29 2023 +0000
 3 files changed, 4 insertions(+), 4 deletions(-)
Auto-merging lib/internal/source_map/source_map_cache.js
[main 6865310b7c] lib: update sourcesToAbsolute comment
 Author: 华 
 Date: Wed Nov 29 17:09:19 2023 +0000
 1 file changed, 1 insertion(+)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)

Executing: git node land --amend --yes
⚠ Found Fixes: #50523, skipping..
--------------------------------- New Message ----------------------------------
lib: don't parse windows drive letters as schemes

We were incorrectly parsing windows drive letters as schemes. This was
polluting the source map cache with malformed file paths.

Fixes: #50523
PR-URL: #50580
Reviewed-By: Chengzhong Wu legendecas@gmail.com

[detached HEAD 0cb57c3f82] lib: don't parse windows drive letters as schemes
Author: 华 sliphua@outlook.com
Date: Mon Nov 6 12:22:41 2023 +0000
4 files changed, 49 insertions(+)
create mode 100644 test/fixtures/source-map/ts-node-win32.js
create mode 100644 test/fixtures/source-map/ts-node.ts
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
lib: typo fixes

PR-URL: #50580
Fixes: #50523
Reviewed-By: Chengzhong Wu legendecas@gmail.com

[detached HEAD 534d96cdaf] lib: typo fixes
Author: 华 sliphua@outlook.com
Date: Sat Nov 25 10:40:29 2023 +0000
3 files changed, 4 insertions(+), 4 deletions(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
lib: update sourcesToAbsolute comment

Co-authored-by: Chengzhong Wu legendecas@gmail.com
PR-URL: #50580
Fixes: #50523
Reviewed-By: Chengzhong Wu legendecas@gmail.com

[detached HEAD 31bd026db9] lib: update sourcesToAbsolute comment
Author: 华 sliphua@outlook.com
Date: Wed Nov 29 17:09:19 2023 +0000
1 file changed, 1 insertion(+)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/7097155769

legendecas added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 5, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 5, 2023
nodejs-github-bot merged commit 77e3dfc into nodejs:main Dec 5, 2023

Copy link
Copy Markdown
Collaborator

Landed in 77e3dfc

RafaelGSS pushed a commit that referenced this pull request Dec 15, 2023
We were incorrectly parsing windows drive letters as schemes. This was
polluting the source map cache with malformed file paths.

Fixes: #50523
PR-URL: #50580
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
RafaelGSS mentioned this pull request Dec 15, 2023
richardlau pushed a commit that referenced this pull request Mar 25, 2024
We were incorrectly parsing windows drive letters as schemes. This was
polluting the source map cache with malformed file paths.

Fixes: #50523
PR-URL: #50580
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
richardlau mentioned this pull request Mar 25, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. source maps Issues and PRs related to source map support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Absolute windows paths are not parsed correctly when generating source map cache

5 participants


Back | FazBrowse Home | New Git URL