FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

esm: remove check for trailing slash during resolve by cyyynthia · Pull Request #57773 · nodejs/node · GitHub

/ node Public

esm: remove check for trailing slash during resolve - #57773

Closed
cyyynthia wants to merge 1 commit into
nodejs:mainfrom
cyyynthia:fix/esm-resolve-stat-ends-with
ConversationCommits (1)ChecksFiles changed
Closed

esm: remove check for trailing slash during resolve#57773
cyyynthia wants to merge 1 commit into
nodejs:mainfrom
cyyynthia:fix/esm-resolve-stat-ends-with

Conversation

cyyynthia commented Apr 6, 2025

Copy link
Copy Markdown

The check was incorrectly altered in #54408, passing the bindings as argument for this instead of the path string. This check not working having no significant impact on Node.js as far as I can tell, this changes removes it altogether to avoid undesired overhead.

nodejs-github-bot commented Apr 6, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Apr 6, 2025

codecov Bot commented Apr 6, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (8456a12) to head (743dba0).
Report is 7 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #57773   +/-   ##
=======================================
  Coverage   90.23%   90.23%           
=======================================
  Files         630      630           
  Lines      185245   185242    -3     
  Branches    36301    36302    +1     
=======================================
+ Hits       167154   167162    +8     
- Misses      11006    11013    +7     
+ Partials     7085     7067   -18
Files with missing lines Coverage Δ
lib/internal/modules/esm/resolve.js 96.26% <100.00%> (+0.08%) ⬆️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cyyynthia force-pushed the fix/esm-resolve-stat-ends-with branch from a12da10 to 743dba0 Compare April 6, 2025 15:05
mcollina requested a review from ljharb April 7, 2025 14:50

mcollina commented Apr 7, 2025

Copy link
Copy Markdown
Member

Can you please add a test?

cyyynthia commented Apr 7, 2025

Copy link
Copy Markdown
Author

I'm unsure what I should be testing specifically; could you perhaps give me some directions to help me get started on that?

ljharb reviewed Apr 7, 2025
View reviewed changes
Comment thread lib/internal/modules/esm/resolve.js Outdated
internalFsBinding,
StringPrototypeEndsWith(internalFsBinding, path, '/') ? StringPrototypeSlice(path, -1) : path,
);
const stats = internalFsBinding.internalModuleStat(internalFsBinding, path);

ljharb Apr 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

failing to strip off the slash seems like it would cause issues on some systems. i'd definitely want to see lots of tests before making such a potentially fundamental change.

cyyynthia Apr 7, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hm I didn't consider the case of other platforms than linux (now that you mention that it is kind of obvious that this is a potential issue).

My PR comes from the fact the check is in a broken state in all v23 releases and in v22.10+, and it doesn't seem to have been a major issue so far. But you're probably right it is best to be cautious and restore the check than to outright get rid of it.

Looking at the history, it appears this check was originally introduced in #34744, for reasons I can't quite grasp. It doesn't come with additional tests for the new code branch either, as far as I can tell.

If it's deemed better to fix and keep the check, I can do that for this PR.

ljharb Apr 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Chesterton's Fence says that before removing anything, one must understand why it's there :-) i think adding test cases would help reveal what purpose it serves.

cyyynthia Apr 11, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Well, it seems I'm not familiar enough with the codebase to confidently navigate around, so I've opted for the alternative option of simply fixing the check, but keeping it there 😅

esm: fix check for trailing slash during resolve
48c9e3c 
The check was incorrectly altered in nodejs#54408, passing the bindings as
argument for `this` instead of the path string.

Signed-off-by: Cynthia Rey <cynthia@cynthia.dev>
cyyynthia force-pushed the fix/esm-resolve-stat-ends-with branch from 743dba0 to 48c9e3c Compare April 11, 2025 08:34
ljharb reviewed Apr 11, 2025
View reviewed changes
Comment thread lib/internal/modules/esm/resolve.js
const stats = internalFsBinding.internalModuleStat(
internalFsBinding,
StringPrototypeEndsWith(internalFsBinding, path, '/') ? StringPrototypeSlice(path, -1) : path,
StringPrototypeEndsWith(path, '/') ? StringPrototypeSlice(path, -1) : path,

ljharb Apr 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this definitely seems like a correct change, because string ends with only takes two arguments, the receiver and the substring - but i still think it needs a test (that would have failed without the fix)

github-actions Bot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

github-actions Bot added stale and removed stale labels Apr 20, 2026

cyyynthia commented Jul 12, 2026

Copy link
Copy Markdown
Author

This patch has been re-filed and merged in #62080; no reason to keep this open 😄

cyyynthia closed this Jul 12, 2026
cyyynthia deleted the fix/esm-resolve-stat-ends-with branch July 12, 2026 11:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ljharb ljharb left review comments

Assignees

No one assigned

Labels

esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL