Begin using `resolve-package-path` for resolve-pkg.

[stefanpenner]

• [x] decide on and verify the appropriate approach for dealing with preserveSymlinks

[stefanpenner]

Seems like the appropriate solution is actually to make this and resolve-package-path actually respect --preserve-symlinks, not force it either way.

opts.preserveSymlinks - if true, doesn't resolve basedir to real path before resolving. This is the way Node resolves dependencies when executed with the --preserve-symlinks flag.

We can also expose preserveSymlinks as an override in resolve-package-path

[davecombs]

I suspect you will lose a fair percentage of the gain of resolve-package-path if preserveSymlinks is set to true, at least in the v-web case, since right now both core and extended just refer up to the common node_modules in v-web. I was actually considering whether it is worthwhile in that case to break the symlinks and just let the resolution algorithm continue up the tree, since it'll arrive at the same place.

[stefanpenner]

@davecombs we unfortunately have no choice, we must respect node's preference or we break --preserve-symlinks

If an application wants to opt-in to that behavior, they can do so via one of the following:

• `NODE_OPTIONS="--preserve-symlinks"
• node --preserve-symlinks

@davecombs if may be that the application in question should opt into those behaviors node-wide, as node itself will also skip the realpaths https://github.com/nodejs/node/blob/a02e3e2d5f1f96f3c408270d45935afdd5d1fffc/lib/internal/modules/esm/default_resolve.js#L79

[davecombs]

I don't actually have an issue with maintaining the symlinks when that's requested by PRESERVE_SYMLINKS (you're right that the original code breaks that, because it specifically forces the keys to be the real paths).

But a question: the code now just returns the filePath when PRESERVE_SYMLINKS is true. However, the intended behavior of the getRealFilePath and getRealDirectoryPath was to return null if the given path did not both exist and point to an entry of the right type (file or directory). The code now seems to lose that guarantee. Or am I missing something? Preserving a symlink seems like one thing, while returning null seems like something else.

[stefanpenner]

was to return null if the given path did not both exist and point to an entry of the right type (file or directory)

I don't understand can you explain further? Specifically what is an example that now misbehaves.

[davecombs]

Sure. Here's the code in _getRealFilePath now, as I have it in my local copy:

function _getRealFilePath(realFilePathCache, filePath) {
  if (realFilePathCache.has(filePath)) {
    return realFilePathCache.get(filePath);  // could be null
  }

  var realPath = null;  // null = 'FILE NOT FOUND'

  try {
    var stat = fs.statSync(filePath);

    // I don't know if Node would handle having the filePath actually
    // be a FIFO, but as the following is also part of the node-resolution
    // algorithm in resolve.sync(), we'll do the same check here.
    if (stat.isFile() || stat.isFIFO()) {
      realPath = fs.realpathSync(filePath);
    }
  } catch (e) {
    if (e === null || typeof e !== 'object' || e.code !== 'ENOENT') {
      throw e;
    }
  }

  realFilePathCache.set(filePath, realPath);

  return realPath;
}

If nothing is found, OR if the file is not actually a file or FIFO, it returns null.

You changed it to:

function _getRealFilePath(realFilePathCache, filePath) {
  if (PRESERVE_SYMLINKS) { return filePath; }

So if PRESERVE_SYMLINKS is true, it doesn't matter if filePath is a real path to a file or not, it's going to be returned. That breaks the original behavior of the function.

[stefanpenner]

@davecombs that makes sense, let me correct

[stefanpenner]

@davecombs good catch, corrected. Now the conditional only wraps the realpath calls themselves.

[davecombs]

Awesome! Thanks.