Revert "path: fix bugs and inconsistencies"

[avivkeller]

This (uncleanly) reverts commit efbba60.

Fixes#55410

---

The commit efbba60 introduced a change where paths would retain trailing / characters. This change led to issues in npm and likely other libraries, because trailing / characters are not typically expected to be preserved.

The purpose of using resolve() on a path is to fully resolve it to its canonical form, regardless of how it is written (e.g., both /hello/../world and /hello/../hello/../world should resolve to the same path). However, this commit altered that behavior, making /hello/ resolve differently from /hello. That made the resolution of identical paths (one with / and one without) return different results. This reverts that behavior

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/url

[codecov]

Codecov Report

Attention: Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (7ae193d) to head (c9d8289).
Report is 65 commits behind head on main.

Files with missing lines	Patch %	Lines
src/path.cc	20.00%	3 Missing and 1 partial ⚠️
lib/internal/url.js	90.00%	0 Missing and 1 partial ⚠️
lib/path.js	96.15%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #55414 +/- ## ========================================== - Coverage 88.40% 88.40% -0.01%  ========================================== Files 653 653 Lines 187600 187518 -82 Branches 36117 36089 -28 ========================================== - Hits 165854 165770 -84 + Misses 14974 14971 -3 - Partials 6772 6777 +5 

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	97.74% <100.00%> (+0.30%)	⬆️
lib/internal/url.js	97.63% <90.00%> (-0.36%)	⬇️
lib/path.js	95.99% <96.15%> (+<0.01%)	⬆️
src/path.cc	69.04% <20.00%> (+1.65%)	⬆️

... and 26 files with indirect coverage changes

[lpinca]

FWIW the npm issue was already reported in the comments of the original PR, see #54224 (comment).

[ovflowd]

FWIW the npm issue was already reported in the comments of the original PR, see #54224 (comment).

I wonder why no one (besides you) followed up on that. It should never have landed. Still, we should have more tests on CITGM to verify these changes.

[ovflowd]

LGTM

[nodejs-github-bot]

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

[huseyinacacak-janea]

Since my PR to fix inconsistencies between functions broke a lot of things in other dependencies, it would be better to revert the PR rather than fix all the other dependencies. Thank you for the PR to revert the changes.

[richardlau]

FWIW the npm issue was already reported in the comments of the original PR, see #54224 (comment).

I wonder why no one (besides you) followed up on that. It should never have landed. Still, we should have more tests on CITGM to verify these changes.

We used to test npm in CITGM, but it often failed (nodejs/citgm#897). I think it was removed as part of one of the clear outs of persistently failing modules.

There's a (stalled?) PR to add it back: nodejs/citgm#979

[vzaidman]

fixes #55424

[RDIL]

This also broke a ton of Yarn's e2e tests, did CITGM miss that too? 🤔

[avivkeller]

For discussion of the CITGM, see nodejs/citgm#1067

[ovflowd]

This also broke a ton of Yarn's e2e tests, did CITGM miss that too? 🤔

I'm not sure if our tests cover it :) https://github.com/nodejs/citgm/tree/main/test/yarn

[RDIL]

Oh, yeah, good point.

[nodejs-github-bot]

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

[avivkeller]

Hey, can this land with the passing CI and approvals?

[bricss]

Looking forward to see 👀 this in the next semver minor or patch release asap ⏰ coz current Node 23 version a little bit broken, atm 🫠

[nodejs-github-bot]

Landed in ee46d22

[avivkeller]

Great! Should this go out in a patch, or in the next minor?

[jkoenig134]

This also broke a ton of Yarn's e2e tests, did CITGM miss that too? 🤔

great question, when will this fix land?

[ljharb]

npm is very broken; it should go out in a patch asap if possible.

[avivkeller]

FWIW I just asked about a potential v23.0.1 patch in slack https://openjs-foundation.slack.com/archives/C019MGJQ8RH/p1729695807771819