preserveSymlinks doesn't work properly from root package

[bterlson]

Consider a library lib with the following file structure:

root
- common
  - node_modules
    - resolve
    - buffer
- mylib
  - node_modules
    - buffer -> ../../common/node_modules/buffer
  - src
    - index.js

In other words, a dependency exists from the root library to buffer which is symlinked into a separate folder (similar to how pnpm works).

In this case, resolve doesn't seem to be mapping the symlinked paths to real paths properly.

Repro Steps

1. Create the above example with mkdir/ln -s
2. cd into root/mylib/src, run node.exe
3. import resolve and observe that resolve('buffer/') returns the symlinked path (root/mylib/node_modules/buffer) instead of the real path (root/common/node_modules/buffer).

[bterlson]

Created a repro repo here: https://github.com/bterlson/resolve-repro

[octogonz]

I'm not sure this is a bug. In your repro, the resolve library does return a path to the correct target file. The path is not normalized, but if you want that, you can normalize it yourself by calling fs.realpathSync(). This is extra disk access, though, and in many cases the caller may not want to do that extra step.

If you can find a repro where resolve returns a path to the /wrong/ file (because it did not traverse symlinks properly), that would certainly be a bug. But I think you'll find that the algorithm is correct, since otherwise a lot of tools would be broken by such a bug. :-)

[bterlson]

@octogonz not sure I follow, but considering the point of this module is to look up modules as Node does, and Node by default (with preserve symlinks off) will never resolve a module to a path which is symlinked, it seems clear this module is behaving incorrectly?

[ljharb]

@octogonz in the “preserve symlinks” case - which node used to do by default - resolve is correct. However, in the “don’t preserve symlinks” case, which is now node’s default as well as will be resolve’s default in v2, it doesn’t seem to be working here (where “working’ is mostly defined as “whatever node does”).

I suspect the reasons this hasn't been reported before include that resolve's preserveSymlinks: false behavior is relatively newly added, and also symlinking is done pretty rarely historically.

[octogonz]

in the “preserve symlinks” case - which node used to do by default - resolve is correct.

Hmmm... If the aim is to mimic the return value of the require.resolve() API from NodeJS, then I agree it should be changed. The fix is pretty easy; call fs.realpathSync() on the return value.

I suspect the reasons this hasn't been reported before include that resolve's preserveSymlinks: false behavior is relatively newly added, and also symlinking is done pretty rarely historically.

Symlinks are used heavily in installations done by PNPM and also Rush, though. By now most tools support that, and rely on the resolve package somehow in their solution. Thus perhaps the explanation is that most callers don't care whether the result is normalized or not. For example, here and here is code that works fine when traversing symlinks, because the next step is to load the file, and it doesn't really matter whether that path is normalized or not.

If we fix this "bug", it might be a good idea to release it as SemVer MINOR (or MAJOR?), since it could theoretically break tools that might have relied on the current behavior somehow.

[ljharb]

Unfortunately every bugfix could potentially break those who rely on the bug - @bterlson is well aware of #194 ;-) - but the alternative is to release every patch as a major, which isn't really sustainable.

Releasing as a minor versus a patch would have no different effect; nobody pins to a minor anymore.

Given that preserveSymlinks is a relatively recent addition - #131 was added in v1.4.0, about 2 years ago compared to v1 being released 5 years ago - and that cases such as you describe will likely continue to Just Work - I think I'm going to end up going for patch.

[bterlson]

@octogonz thank you so much for finding usage examples! It does seem possible that depending on this symlinking behavior is rare if most tools only need to a valid node_modules folder for io and don't care about the precise path. I thought for a while about how this could break things (I owe it to past me heh) but the current path normalization behavior seems hard to rely on. That said, I still expect this issue to get a ton of traffic in a few months :-P

FWIW, I hit this because rollup is sensitive to path normalization. Resolving the same library to different paths means different copies of the library in the bundle. Resolve (in the context of rollup-plugin-node-resolve) seems to do the right thing most of the time, so it's possible this has just gone unnoticed for a while.

[octogonz]

Releasing as a minor versus a patch would have no different effect; nobody pins to a minor anymore.

I feel like we... come very different worlds. Turns out the two libraries that I referenced use ~ by default and have actually locked resolve to a PATCH number "for some reason." 😁

  "dependencies": {
    "@microsoft/api-extractor-model": "7.2.0",
    "@microsoft/node-core-library": "3.13.0",
    "@microsoft/ts-command-line": "4.2.6",
    "@microsoft/tsdoc": "0.12.9",
    "colors": "~1.2.1",
    "lodash": "~4.17.5",
    "resolve": "1.8.1",
    "source-map": "~0.6.1",
    "typescript": "~3.4.3"
  },

[ljharb]

Pinning dependencies in package.json to exact versions doesn't actually give you any guarantees; you need a lockfile for that, and it's been the best practice in node for many many years to use ^ (except for known bugs, of course).

That said, I certainly don't want to break anyone - but this feels like it's worth shipping it as a bugfix and waiting to get feedback.

[octogonz]

I'll point out that a lockfile doesn't help new installs that happen after a regression occurs, but before the problem is found+fixed. And in this case (since resolve doesn't have any indirect dependencies), locking the version does indeed prevent your fix from getting installed before it's been properly tested.

But I suppose it also means releasing as PATCH is cool as far as these two examples go heheh.

[ljharb]

See #197.