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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Dec 5, 2023
Merged

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

merged 3 commits into from
Dec 5, 2023

Conversation

PaperStrike
Copy link
Contributor

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

Fixes: #50523

@PaperStrike
lib: don't parse windows drive letters as schemes
64365ad 
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 nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 6, 2023
@PaperStrike
Copy link
Contributor Author

@bcoe @cspotcode Any thoughts? Thanks in advance.

bcoe
bcoe reviewed Nov 21, 2023
View reviewed changes
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

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

@PaperStrike
Copy link
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

tniessen
tniessen reviewed Nov 24, 2023
View reviewed changes
test/parallel/test-source-map-enable.js Outdated Show resolved Hide resolved
test/fixtures/source-map/ts-node-win32.js Outdated Show resolved Hide resolved
@legendecas
Copy link
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 legendecas added the source maps Issues and PRs related to source map support. label Nov 24, 2023
legendecas
legendecas reviewed Nov 29, 2023
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM with the comment change

lib/internal/source_map/source_map_cache.js Show resolved Hide resolved
@PaperStrike @legendecas
lib: update sourcesToAbsolute comment
f41d758 
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 3, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@PaperStrike
Copy link
Contributor Author

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

@nodejs-github-bot
Copy link
Collaborator

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

@legendecas legendecas added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 5, 2023
@legendecas
Copy link
Member

Thank you for your contribution!

@nodejs-github-bot 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
@nodejs-github-bot
Copy link
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 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 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 nodejs-github-bot merged commit 77e3dfc into nodejs:main Dec 5, 2023
65 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 77e3dfc

RafaelGSS pushed a commit that referenced this pull request Dec 15, 2023
@PaperStrike @RafaelGSS
lib: don't parse windows drive letters as schemes
7a8a2d5 
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 RafaelGSS mentioned this pull request Dec 15, 2023
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@PaperStrike @richardlau
lib: don't parse windows drive letters as schemes
5c7a9c8 
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 richardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen left review comments

@bcoe bcoe bcoe left review comments

@legendecas legendecas legendecas approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Absolute windows paths are not parsed correctly when generating source map cache
5 participants
@PaperStrike @legendecas @nodejs-github-bot @bcoe @tniessen