esm: remove check for trailing slash during resolve #57773
Conversation
|
Review requested:
|
nodejs-github-bot
added
esm
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
🚀 New features to boost your workflow:
|
cyyynthia
force-pushed
the
fix/esm-resolve-stat-ends-with
branch
from
a12da10 to
743dba0
Compare
|
Can you please add a test? |
|
I'm unsure what I should be testing specifically; could you perhaps give me some directions to help me get started on that? |
lib/internal/modules/esm/resolve.js
Outdated
| internalFsBinding, | ||
| StringPrototypeEndsWith(internalFsBinding, path, '/') ? StringPrototypeSlice(path, -1) : path, | ||
| ); | ||
| const stats = internalFsBinding.internalModuleStat(internalFsBinding, path); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
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 <[email protected]>
cyyynthia
force-pushed
the
fix/esm-resolve-stat-ends-with
branch
from
743dba0 to
48c9e3c
Compare
| @@ -249,7 +249,7 @@ function finalizeResolution(resolved, base, preserveSymlinks) { | |||
|
|
|||
| const stats = internalFsBinding.internalModuleStat( | |||
| internalFsBinding, | |||
| StringPrototypeEndsWith(internalFsBinding, path, '/') ? StringPrototypeSlice(path, -1) : path, | |||
| StringPrototypeEndsWith(path, '/') ? StringPrototypeSlice(path, -1) : path, | |||
There was a problem hiding this comment.
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)
The check was incorrectly altered in #54408, passing the bindings as argument for
thisinstead 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.