GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — module,win: fix long path resolve by huseyinacacak-janea · Pull Request #53294 · nodejs/node Conversation
Contributor
Several issues were encountered with module loading due to extended path lengths on Windows. These have been addressed with corrections implemented across four distinct code locations, accompanied by the addition of a corresponding test case for each to ensure functionality. Fixes: #50753
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
huseyinacacak-janea
force-pushed
the
huseyin-11801-long-path-error
branch
from
3ee742c to
27a4505
Compare
H4ad
previously approved these changes
H4ad
dismissed
their stale review
I actually need more time to take a look, I think the implementation will solve the problem but I want to test in which cases we should include the UNC path, since we didn't do this in all paths in the toNamespacedPath.
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
anonrig
requested changes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to land this after the toNamespacedPath migration to C++. Unfortunately, I don't have the time to pursue this at the moment (afaict, there is only one failing test). I appreciate if you could took over that PR and apply these changes in a follow up PR.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
huseyinacacak-janea
force-pushed
the
huseyin-11801-long-path-error
branch
from
27a4505 to
42b88bf
Compare
huseyinacacak-janea
force-pushed
the
huseyin-11801-long-path-error
branch
from
42b88bf to
14a1de0
Compare
H4ad
approved these changes
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
anonrig
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Thanks.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
anonrig
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous line is not needed Can you also remove this in other places?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
anonrig
reviewed
Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
anonrig
approved these changes
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
targos
pushed a commit
that referenced
this pull request
PR-URL: #53294 Fixes: #50753 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Merged
This was referenced
17 tasks done
DanielForniessoria-TomTom
added a commit
to tomtom-international/tomtom-traffic-analytics-mcp
that referenced
this pull request
The old comment blamed a Node bug: package.json read silently failing past
MAX_PATH, so `{"type":"module"}` was missed and ESM parsed as CJS. That was
fixed in Node 22.7.0 (nodejs/node#53294, which namespaces fs, ESM resolution
and package.json reads) and this package requires >=22.9.0, so the mechanism
the comment described cannot be why we need the setting.
The reason that does hold up is headroom, measured on this lockfile: the
longest path relative to the repo root is 115 characters hoisted against 190
isolated (at Windows' default virtualStoreDirMaxLength of 60). An ordinary
C:\Users\<first>.<last>\IdeaProjects\tomtom-traffic-analytics-mcp checkout is
63 characters, leaving isolated ~6 before MAX_PATH — a OneDrive-redirected
Documents folder or a worktree subdirectory exceeds it. Enabling long paths
instead needs an admin registry value plus a longPathAware manifest node.exe
does not ship (nodejs/node#40641), which a public repo cannot require.
Also records what the setting does and does not cost, since that was the
review question. It costs strict isolation: an undeclared dependency resolves
locally but breaks in the published bundle. It costs nothing measurable
otherwise — 321 MB and equal install times either way (isolated 4.86/6.65/
5.12s against hoisted 5.82/5.77/5.11s, warm store, frozen lockfile), because
the store dedup comes from packageImportMethod, not from nodeLinker.
Comment only; no behaviour change.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page. AssigneesNo one assignedLabels
c++
ProjectsMilestoneNo milestoneDevelopmentSuccessfully merging this pull request may close these issues. Long node_modules paths cannot be found on Windows whenLongPathsEnabled enabled
Uh oh!
There was an error while loading. Please reload this page. Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| You are visitor 01402503 — GeezerVison does not store the pages you look up. Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required. |