Node: Regression: Resolution algorithm collision

[bmeck]

• 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.

[jasnell]

ping @nodejs/ctc

[cjihrig]

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]

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]

As far as I see it the test (and code change) exists since iojs 1.0 and was introduced here https://github.com/nodejs/node/commit/36777d2a5f4f8da938edf9290fb74e381bfdb644

[bmeck]

Any way we look at it, we should fix the require("./foo") example above

[jasnell]

I'm +1 for reverting but we need to take a close look at the semver-i-iness of the change.

[bnoordhuis]

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 https://github.com/nodejs/node/issues/4595#issuecomment-170143725 for an example.

[jasnell]

@bnoordhuis ... given that, which do you prefer: modifying the docs to match the impl or modifying the impl to match the docs?

[bnoordhuis]

The path of least resistance is to update the documentation; any change to the algorithm will almost certainly result in some fallout.

[bmeck]

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" [e-mail] 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
https://github.com/nodejs/node/issues/14990#issuecomment-324436758, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAOUo663MpquVS8NKEyOS12g0P4Tq8p9ks5sbHwbgaJpZM4PAP2W
.

[bmeck]

It should be noted that userland implementations of resolution logic like resolve at 800k downloads per day don't exhibit this behavior.

[mcollina]

@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]

From a user perspective (and resolve maintainer perspective), the current behavior is very very strange, and I'd vastly prefer the (at least) <= 0.8 behavior.

[bmeck]

Made a PR people an look at.

[mcollina]

@bmeck do you have a trail on why this change was introduced?

[bmeck]

@mcollina Unclear on a few quick searches, but the test makes it seem to be because someone had the following:

foo/
   package.json -> main.js
   main.js
   bar/
      baz.js -> require('../..')
foo.js

And baz resolved ../.. to foo.js instead of foo/main.js

[mcollina]

I do not think we can change that behavior now.

[bmeck]

@mcollina no userland impl uses this odd behavior.

[mcollina]

@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]

@mcollina to preserve the nature of the test, we could make trailingSlash set to true whenever resolving a path ending in /. or /..

[bmeck]

if it matters URL adds trailing slashes:

console.log(new URL('../..', 'file://a/b/c/d').href); // file://a/

[mcollina]

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. trailingSlash  set to true where?

[bmeck]

@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 trailingSlash is 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.js

If baz.js does require('..'), it would not find foo.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]

https://github.com/nodejs/node/issues/14990#issuecomment-324929142 I am in favor of what you propose there.

Is that related in https://github.com/nodejs/node/pull/15015? Over there it mentions package.json, but not in the comment above.

[evanlucas]

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?