path: update win32 toNamespacedPath to support device namespace paths by injunchoi98 · Pull Request #54367 · nodejs/node · GitHub

Updated the win32 toNamespacedPath function to support device namespace paths as per #54180 and #54161.
url.pathToFileURL was not modified since device namespace paths are not valid file URIs.

Codecov Report

❌ Patch coverage is 85.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.23%. Comparing base (880c446) to head (9eb494a).
⚠️ Report is 5059 commits behind head on main.

Files with missing lines	Patch %	Lines
src/path.cc	57.14%	6 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54367      +/-   ##
==========================================
+ Coverage   87.09%   88.23%   +1.14%     
==========================================
  Files         648      651       +3     
  Lines      182216   183895    +1679     
  Branches    34965    35847     +882     
==========================================
+ Hits       158704   162267    +3563     
+ Misses      16785    14913    -1872     
+ Partials     6727     6715      -12

Files with missing lines	Coverage Δ
lib/path.js	95.61% <100.00%> (+0.44%)	⬆️
src/path.cc	67.39% <57.14%> (-1.66%)	⬇️

... and 248 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Would it be faster if the object was an array?

Yes, you're correct. My local benchmarks confirm that using an array is indeed faster than using an object. Thank you very much for your thorough review. I will take this and apply the changes accordingly

Suggested change

      
            // COM to AUX (e.g., COM1, LPT1, NUL, CON, PRN, AUX) are reserved OS device names.
          
            //
          
            // COM to AUX (e.g., COM1, LPT1, NUL, CON, PRN, AUX) are reserved OS device names.

This might help to separate the ideas

You're absolutely right. Moreover, the code I recently added is currently checking all regular expressions in the device namespace, which has significantly reduced performance. As you suggested, I will try to improve performance by distinguishing between special devices and reserved devices during the inspection. Thank you.

I added comments to differentiate between reserved and non-reserved device names, explaining the differences between regular expressions like /([\\/])?(COM\d+)$/i and /^(PHYSICALDRIVE\d+)$/i. While I tried to maintain performance while addressing the issues, I couldn't find a way to do so effectively because reserved device names can also appear in paths (e.g., C:/path/COM1). If anyone has a better solution that maintains performance while resolving this, I welcome suggestions.

I believe this is already being fixed in #54224

Thank you for let me know sir. I'll close this PR

sir Im bit confused that #54224 seems to be focused on fixing an issue where a trailing backslash was incorrectly added in the resolve function, whereas my PR is specifically aimed at converting file paths to namespace paths in the tonamespaced function. It seems these are addressing different concerns..?

I would work on this issue after finishing my current work. So, I reviewed this PR.

This PR fixes a different problem than #54224 and LGTM.

I think the author can continue working on this by reopening it.

I believe this PR could fix #54161 as well. The conout$ and conin$ could be added as regular expressions. What do you think?
Refs: https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file
Refs: https://learn.microsoft.com/en-us/windows/console/console-handles

Thank you for your valuable feedback. I initially included only CON as a regular expression, but I completely agree that conout$ and conin$ should also be added as regular expressions. I will reopen this PR tomorrow. I appreciate your guidance.

Note that toNamespacedPath also has a C++ implementation that would have to be updated as well.

Thank you for let me know. I just updated C++ implementation as well.

@targos I made the changes based on your feedback. Could you review this PR when you have a moment?

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

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

Could you update the doc also?

Of course. Thank you for let me know. I updated the doc also according to your feedback.

Hi, is the original issue still relevant? I was in the process of backporting #52135 which seems to have caused some regressions and it seems this is fixing one of the them that are still around?

The original issue (the lack of support for the device namespace in toNamespace on Windows) still seems unresolved. However, if #52135 modified the C++ implementation of toNamespace, it seems likely that the C++ portion of this PR would also need adjustments.

nodejs-github-bot added needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. labels Aug 14, 2024

avivkeller added the windows Issues and PRs related to the Windows platform. label Aug 14, 2024

avivkeller reviewed Aug 14, 2024

View reviewed changes

injunchoi98 force-pushed the namedspace branch 2 times, most recently from 5000736 to d347f33 Compare August 14, 2024 12:29

avivkeller added the needs-benchmark-ci PR that need a benchmark CI run. label Aug 14, 2024

avivkeller reviewed Aug 14, 2024

View reviewed changes

injunchoi98 closed this Aug 15, 2024

injunchoi98 deleted the namedspace branch August 15, 2024 07:23

injunchoi98 restored the namedspace branch August 16, 2024 01:05

injunchoi98 reopened this Aug 16, 2024

injunchoi98 force-pushed the namedspace branch 2 times, most recently from 9b397f6 to aed9a78 Compare August 16, 2024 01:45

injunchoi98 force-pushed the namedspace branch from aed9a78 to ee89dc7 Compare August 17, 2024 09:27

injunchoi98 force-pushed the namedspace branch 2 times, most recently from 09f9091 to 0f8415f Compare August 19, 2024 01:55

injunchoi98 mentioned this pull request Aug 28, 2024

CONOUT$ no longer available for Node 22? #54161

Closed

2 tasks

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2024

daeyeon reviewed Aug 31, 2024

View reviewed changes

Comment thread lib/path.js Outdated

Comment thread src/path.cc Outdated

injunchoi98 force-pushed the namedspace branch from 0f8415f to fb92220 Compare September 1, 2024 08:13

daeyeon approved these changes Sep 1, 2024

View reviewed changes

daeyeon added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Sep 1, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2024

daeyeon removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 2, 2024

injunchoi98 force-pushed the namedspace branch 4 times, most recently from 19f95e2 to fb05e6e Compare September 24, 2024 09:51

daeyeon added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Sep 25, 2024

injunchoi98 force-pushed the namedspace branch from fb05e6e to 15ecd34 Compare September 25, 2024 05:04

injunchoi98 force-pushed the namedspace branch from 15ecd34 to e04d346 Compare September 26, 2024 04:34

path: update win32 toNamespacedPath to support device namespace paths

9eb494a

injunchoi98 force-pushed the namedspace branch from e04d346 to 9eb494a Compare September 26, 2024 04:39

joyeecheung mentioned this pull request Jan 23, 2025

[v20.x backport] src: move package_json_reader cache to c++ #56590

Closed

@@ @@ -687,6 +707,19 @@ const win32 = { @@
                   if (typeof path !== 'string' || path.length === 0)
                     return path;
+                  // Check if the path matches any device pattern
+                  if (ObjectValues(windowDevicePatterns).some((pattern) => pattern.test(path))) {

@@ @@ -180,6 +180,25 @@ function glob(path, pattern, windows) { @@
                 });
               }
+              // Regular expressions to identify special device names in Windows.
+              // Ref: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea
+              // COM to AUX (e.g., COM1, LPT1, NUL, CON, PRN, AUX) are reserved OS device names.

	// COM to AUX (e.g., COM1, LPT1, NUL, CON, PRN, AUX) are reserved OS device names.
	//
	// COM to AUX (e.g., COM1, LPT1, NUL, CON, PRN, AUX) are reserved OS device names.

path: update win32 toNamespacedPath to support device namespace paths by injunchoi98 · Pull Request #54367 · nodejs/node · GitHub

Uh oh!

Uh oh!

path: update win32 toNamespacedPath to support device namespace paths - #54367

path: update win32 toNamespacedPath to support device namespace paths#54367injunchoi98 wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom injunchoi98:namedspaceinjunchoi98/node:namedspaceCopy head branch name to clipboard

Conversation

injunchoi98 commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

avivkeller Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

injunchoi98 Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

avivkeller Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

injunchoi98 Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

injunchoi98 Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

targos commented Aug 15, 2024

Uh oh!

injunchoi98 commented Aug 15, 2024

Uh oh!

injunchoi98 commented Aug 15, 2024

Uh oh!

huseyinacacak-janea commented Aug 15, 2024

Uh oh!

huseyinacacak-janea commented Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

injunchoi98 commented Aug 15, 2024

Uh oh!

targos commented Aug 16, 2024

Uh oh!

injunchoi98 commented Aug 17, 2024

Uh oh!

injunchoi98 commented Aug 24, 2024

Uh oh!

nodejs-github-bot commented Aug 29, 2024

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 1, 2024

Uh oh!

daeyeon commented Sep 25, 2024

Uh oh!

injunchoi98 commented Sep 25, 2024

Uh oh!

joyeecheung commented Jan 23, 2025

Uh oh!

injunchoi98 commented Jan 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

path: update win32 toNamespacedPath to support device namespace paths#54367
injunchoi98 wants to merge 1 commit into
nodejs:mainfrom
injunchoi98:namedspace

injunchoi98 commented Aug 14, 2024 •
edited
Loading

codecov Bot commented Aug 14, 2024 •
edited
Loading

huseyinacacak-janea commented Aug 15, 2024 •
edited
Loading