path: update win32 toNamespacedPath to support device namespace paths #54367
Conversation
nodejs-github-bot added needs-ci 
Codecov ReportAttention: Patch coverage is
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
|
lib/path.js Outdated
| return path; | ||
| | ||
| // Check if the path matches any device pattern | ||
| if (ObjectValues(windowDevicePatterns).some((pattern) => pattern.test(path))) { |
There was a problem hiding this comment.
Would it be faster if the object was an array?
There was a problem hiding this comment.
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
injunchoi98 force-pushed the namedspace branch 2 times, most recently from 5000736 to d347f33 Compare lib/path.js Outdated
| | ||
| // 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. |
There was a problem hiding this comment.
| // 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
| 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 |
| Thank you for your valuable feedback. I initially included only |
injunchoi98 force-pushed the namedspace branch 2 times, most recently from 9b397f6 to aed9a78 Compare | Note that |
injunchoi98 force-pushed the namedspace branch from aed9a78 to ee89dc7 Compare | Thank you for let me know. I just updated C++ implementation as well. |
injunchoi98 force-pushed the namedspace branch 2 times, most recently from 09f9091 to 0f8415f Compare | @targos I made the changes based on your feedback. Could you review this PR when you have a moment? |
github-actions bot removed the request-ci 
injunchoi98 force-pushed the namedspace branch from 0f8415f to fb92220 Compare 
daeyeon added request-ci github-actions bot removed the request-ci daeyeon removed the author ready injunchoi98 force-pushed the namedspace branch 4 times, most recently from 19f95e2 to fb05e6e Compare 
| Could you update the doc also? |
daeyeon added semver-minor injunchoi98 force-pushed the namedspace branch from fb05e6e to 15ecd34 Compare | Of course. Thank you for let me know. I updated the doc also according to your feedback. |
injunchoi98 force-pushed the namedspace branch from 15ecd34 to e04d346 Compare 
injunchoi98 force-pushed the namedspace branch from e04d346 to 9eb494a Compare 
| 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. |
7 participants
Updated the win32
toNamespacedPathfunction to support device namespace paths as per #54180 and #54161.url.pathToFileURLwas not modified since device namespace paths are not valid file URIs.