Repository navigation
Regression: Resolution algorithm collision #14990
Description
Activity
ping @nodejs/ctc
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.
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.
Any way we look at it, we should fix the
require("./foo")example aboveI'm +1 for reverting but we need to take a close look at the semver-i-iness of the change.
The test is invalid since it doesn't match the docs.
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.
@bnoordhuis ... given that, which do you prefer: modifying the docs to match the impl or modifying the impl to match the docs?
The path of least resistance is to update the documentation; any change to the algorithm will almost certainly result in some fallout.
- How do we cleanly explain that it searches for directory package.json before file extensions but directory index files after file extensions. Mind you the change in the past was also breaking.…On Aug 23, 2017 2:23 PM, "Ben Noordhuis" ***@***.***> wrote: The path of least resistance is to update the documentation; any change to the algorithm will almost certainly result in some fallout. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14990 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOUo663MpquVS8NKEyOS12g0P4Tq8p9ks5sbHwbgaJpZM4PAP2W> .Reacted by Mariusz Nowak
@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.
Reacted by James M SnellFrom 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 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.
if it matters
URLadds trailing slashes:console.log(new URL('../..', 'file://a/b/c/d').href); // file://a/
to preserve the nature of the test, we could make trailingSlash set to true whenever resolving a path ending in /. or /..
@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?@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:
foo/ bar/ baz.js foo.jsIf
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.#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.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?
- added a commit that references this issue
on Feb 17, 2018 - added a commit that references this issue
on Mar 11, 2018 - added a commit that references this issue
on May 8, 2018
At some time just before v4 a seemingly invalid test was introduced. It invalidates the resolve algorithm as documented by checking for
./foo/package.jsonbefore./foo.jswhen usingrequire('./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.