Repository navigation
Regression: Resolution algorithm collision #14990
Description
- Version: v4+
- Platform: all
- Subsystem: module
At some time just before v4 a seemingly invalid test was introduced. It invalidates the resolve algorithm as documented by checking for ./foo/package.json before ./foo.js when using require('./foo') always.
The intent was for require("..") to prefer a directory instead of doing regular resolution. However, that does not match the documented algorithm which would require a / to invalidate file searching.
I don't have a clear way to explain the current behavior and would like to revert this behavior. We could state that matching /${path_separator}.?./$ at the end of a require specifier would automatically add / but that seems a bit odd.
I want to revert this change.
Activity
jasnell commentedon Aug 23, 2017
ping @nodejs/ctc
cjihrig commentedon Aug 23, 2017
Is it just a test, or were there changes to the algorithm itself? If it's just a test, could we rework it to be correct? If there were changes to the algorithm, then we might want to update the docs instead.
bmeck commentedon Aug 23, 2017
The test is invalid since it doesn't match the docs. We need to revert to the original algorithm to fix. I don't want to update docs because the behavior is very odd to me.
BridgeAR commentedon Aug 23, 2017
As far as I see it the test (and code change) exists since iojs 1.0 and was introduced here 36777d2
bmeck commentedon Aug 23, 2017
Any way we look at it, we should fix the
require("./foo")example abovejasnell commentedon Aug 23, 2017
I'm +1 for reverting but we need to take a close look at the semver-i-iness of the change.
bnoordhuis commentedon Aug 23, 2017
The description in the documentation has been at odds with the implementation since at least v0.12, possibly v0.10. See #4595 (comment) for an example.
jasnell commentedon Aug 23, 2017
@bnoordhuis ... given that, which do you prefer: modifying the docs to match the impl or modifying the impl to match the docs?
bnoordhuis commentedon Aug 23, 2017
The path of least resistance is to update the documentation; any change to the algorithm will almost certainly result in some fallout.
bmeck commentedon Aug 23, 2017
bmeck commentedon Aug 24, 2017
It should be noted that userland implementations of resolution logic like resolve at 800k downloads per day don't exhibit this behavior.
mcollina commentedon Aug 24, 2017
@bmeck if that's not too much effort, would you assemble a PR anyway? It is hard to understand the impact of this behaviour without a SHA reference and a full example. I think seeing the actual change you are planning to make would help.
ljharb commentedon Aug 24, 2017
From a user perspective (and
resolvemaintainer perspective), the current behavior is very very strange, and I'd vastly prefer the (at least) <= 0.8 behavior.4 remaining items
bmeck commentedon Aug 24, 2017
@mcollina no userland impl uses this odd behavior.
mcollina commentedon Aug 24, 2017
@bmeck definitely. However I fear there is plenty of code that is using that behavior, even in npm and also closed source. This makes it semver-major squared for me, and maybe the ship of changing this has sailed.
Who did the original edits? I would ask to @isaacs what he thinks of this potential change.
bmeck commentedon Aug 24, 2017
@mcollina to preserve the nature of the test, we could make
trailingSlashset to true whenever resolving a path ending in/.or/..bmeck commentedon Aug 24, 2017
if it matters
URLadds trailing slashes:mcollina commentedon Aug 25, 2017
@bmeck is this a suggestion to change the behavior or just amend the test?
Bear with me, you are way more familiar with the internal of the module system than myself, and I care that the current behavior is not changed, as I fear breakages.
trailingSlashset totruewhere?bmeck commentedon Aug 25, 2017
@mcollina in all scenarios, the behavior needs to change due to inconsistencies. See the test in the PR pointing to this issue for an example.
The suggestion above about setting
trailingSlashis to make the old test pass while fixing inconsistencies. It would, however, add a minor documentation change and not act 100% the same as before the regression occured.In particular given the dir structure:
If
baz.jsdoesrequire('..'), it would not findfoo.js, which it currently does find (but the regression commit/test seems to revolve around loading directory instead of files when using..).The behavior of approximately adding a trailing
/is nice however since it matched what the URL specification does when resolving with.and..specifiers. In those cases the URL specification keeps the trailing slash, while node's resolution currently removes the trailing slash.mcollina commentedon Sep 13, 2017
#14990 (comment) I am in favor of what you propose there.
Is that related in #15015? Over there it mentions
package.json, but not in the comment above.evanlucas commentedon Dec 6, 2017
I think we should revert to the old (and documented) behavior. If there is concern about the ecosystem effects, we could allow it to be an opt-out with a flag?
module: check file ext before dir as documented
module: check file ext before dir as documented
module: check file ext before dir as documented