| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Can we add a test so we don't regress?
Sorry, something went wrong.
|
FYI @nodejs/loaders @nodejs/fs |
Sorry, something went wrong.
Of course. I'll add it next week. |
Sorry, something went wrong.
Test's added. |
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
|
@joyeecheung do you think this can land as a fix with a regression test added and then a follow-up PR can be opened for porting path.toNamespacedPath() logic and using it in here, but potentially in other places missing it as well? |
Sorry, something went wrong.
|
Yes, with a TODO/FIXME comment. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
@joyeecheung can you add the commit-queue label to try to land this again? Last time it failed to do so... |
Sorry, something went wrong.
Sorry, something went wrong.
|
@StefanStojanovic @joyeecheung any chance of getting this released in one of the next patch or minor releases? It's been a while since my original issue in Nov 2023 and users keep running into this (eg. with pnpm) and commenting that it's an open bug but the PR hasn't been merged yet. |
Sorry, something went wrong.
Commit Queue failed- Loading data for nodejs/node/pull/51097 ✔ Done loading data for nodejs/node/pull/51097 ----------------------------------- PR info ------------------------------------ Title module,win: fix long path resolve (#51097) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch StefanStojanovic:mefi-resolve-long-path -> nodejs:main Labels c++, fs, needs-ci Commits 1 - module,win: fix long path resolve Committers 1 - StefanStojanovic PR-URL: https://github.com/nodejs/node/pull/51097 Fixes: https://github.com/nodejs/node/issues/50753 Reviewed-By: Joyee Cheung ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/51097 Fixes: https://github.com/nodejs/node/issues/50753 Reviewed-By: Joyee Cheung -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - module,win: fix long path resolve ℹ This PR was created on Fri, 08 Dec 2023 13:28:00 GMT ✔ Approvals: 1 ✔ - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/51097#pullrequestreview-1891732903 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-04-01T17:51:22Z: https://ci.nodejs.org/job/node-test-pull-request/58036/ - Querying data for job/node-test-pull-request/58036/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/8596048782 |
Sorry, something went wrong.
@joyeecheung do you see why this couldn't land from this log? |
Sorry, something went wrong.
Sorry, something went wrong.
|
It looks like the Node.js v22 PR contains the fix "module,win: fix long path resolve (by @StefanStojanovic) #51097": So maybe Node.js has support for long paths on Windows now! (make sure that you have LongPathsEnabled set to true on the Windows machine you test on) |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
As reported here, there is a problem loading modules from long paths on Windows. This change fixes it by adding the required \\?\ prefix, with which, resolving long paths will work regardless of whether the LongPathsEnabled value is set or not.
Fixes: #50753