Revert "path: fix bugs and inconsistencies" by avivkeller · Pull Request #55414 · nodejs/node

avivkeller · 2024-10-17T01:14:56Z

This (uncleanly) reverts commit efbba60.

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 · 2024-10-17T01:15:01Z

Review requested:

@nodejs/loaders
@nodejs/url

codecov · 2024-10-17T02:31:04Z

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 · 2024-10-17T06:58:11Z

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

ovflowd · 2024-10-17T07:38:54Z

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 · 2024-10-17T11:12:28Z

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

huseyinacacak-janea · 2024-10-17T11:59:09Z

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 · 2024-10-17T12:32:32Z

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 · 2024-10-17T14:33:01Z

fixes #55424

RDIL · 2024-10-17T14:35:00Z

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

avivkeller · 2024-10-17T14:36:29Z

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

ovflowd · 2024-10-17T14:52:38Z

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 · 2024-10-17T14:54:38Z

Oh, yeah, good point.

nodejs-github-bot · 2024-10-18T23:19:57Z

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

avivkeller · 2024-10-19T23:16:07Z

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

bricss · 2024-10-20T10:33:11Z

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 · 2024-10-21T07:10:48Z

Landed in ee46d22

avivkeller · 2024-10-21T12:33:34Z

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

jkoenig134 · 2024-10-23T14:52:14Z

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

great question, when will this fix land?

ljharb · 2024-10-23T14:59:08Z

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

avivkeller · 2024-10-23T15:05:25Z

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

This reverts commit efbba60. PR-URL: #55414 Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

avivkeller added revert PRs that revert previously landed PRs. and removed c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Oct 17, 2024

SuperchupuDev mentioned this pull request Oct 17, 2024
Upstream node 23 issue makes absolute: true not work SuperchupuDev/tinyglobby#62
Closed

ovflowd approved these changes Oct 17, 2024
View reviewed changes

avivkeller added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 17, 2024

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 17, 2024

richardlau approved these changes Oct 17, 2024
View reviewed changes

bricss mentioned this pull request Oct 17, 2024
Node 23 - heap out of memory #55418
Closed

This was referenced Oct 17, 2024
Path.relative breaks for paths that differ by trailing separator in v23.0.0 #55424
Closed
path: fix bugs and inconsistencies #54224
Merged

This was referenced Oct 18, 2024
[Bug]: Node 23 normalizes paths in a way that results in Jest finding no tests jestjs/jest#15348
Closed
Temporarily disable Node 23 CI tests solana-foundation/solana-web3.js#3395
Merged

Bo98 mentioned this pull request Oct 19, 2024
node 23.1.0 Homebrew/homebrew-core#194645
Merged
1 task

lpinca approved these changes Oct 21, 2024
View reviewed changes

lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 21, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 21, 2024

nodejs-github-bot merged commit ee46d22 into nodejs:mainOct 21, 2024

mryhryki mentioned this pull request Oct 21, 2024
Fix CI on Node.js v23.0.0 mryhryki/simple-encryption#51
Closed

aladdin-add mentioned this pull request Oct 22, 2024
ci: Add node 23 to ci workflows eslint-community/eslint-plugin-n#368
Merged

clemyan mentioned this pull request Oct 22, 2024
[Bug?]: Error "Required package missing from disk" when building with Node 23.0.0 yarnpkg/berry#6570
Open
1 task

aduh95 mentioned this pull request Oct 24, 2024
V23.1.0 proposal #55513
Merged

scagood mentioned this pull request Oct 24, 2024
Regression when updating to node: ^23 (no-unpublished-require) eslint-community/eslint-plugin-n#369
Closed

richardlau added dont-land-on-v18.x dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Oct 29, 2024

ronso0 mentioned this pull request Nov 5, 2024
chore: pin Node.JS pre-commit hooks to use LTS mixxxdj/mixxx#13785
Merged

priyagupta108 mentioned this pull request Nov 14, 2024
Yarn installs failing in GitHub Actions after updating to Node 23.0.0 actions/setup-node#1157
Closed
5 tasks

Flarna mentioned this pull request Nov 30, 2024
Revert of nodejs/node#55623 #56145
Closed

huseyinacacak-janea mentioned this pull request Dec 2, 2024
fs,win: fix readdir for named pipe #56110
Merged

Uh oh!

Uh oh!

Conversation

avivkeller commented Oct 17, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 17, 2024

Uh oh!

codecovBot commented Oct 17, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lpinca commented Oct 17, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ovflowd commented Oct 17, 2024

Uh oh!

ovflowd left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 17, 2024

Uh oh!

huseyinacacak-janea commented Oct 17, 2024

Uh oh!

richardlau commented Oct 17, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vzaidman commented Oct 17, 2024

Uh oh!

RDIL commented Oct 17, 2024

Uh oh!

avivkeller commented Oct 17, 2024

Uh oh!

ovflowd commented Oct 17, 2024

Uh oh!

RDIL commented Oct 17, 2024

Uh oh!

nodejs-github-bot commented Oct 18, 2024

Uh oh!

avivkeller commented Oct 19, 2024

Uh oh!

bricss commented Oct 20, 2024

Uh oh!

nodejs-github-bot commented Oct 21, 2024

Uh oh!

avivkeller commented Oct 21, 2024

Uh oh!

jkoenig134 commented Oct 23, 2024

Uh oh!

ljharb commented Oct 23, 2024

Uh oh!

avivkeller commented Oct 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

avivkeller commented Oct 17, 2024•
edited
Loading

codecovBot commented Oct 17, 2024•
edited
Loading

lpinca commented Oct 17, 2024•
edited
Loading

richardlau commented Oct 17, 2024•
edited
Loading