node resolution algorithm with NODE_PATH resolves in wrong order

[mzeiher]

Hi

I found a bug in the resolution algorithm, node_modules folders in the same directory and in parent directories should have precedence over modules directly in NODE_PATH

here is a repository to reproduce this bug:
https://github.com/mzeiher/esbuild-node-path-bug

there are 2 versions of "package-c" in the directory which is loaded via NODE_PATH, one which exports "version 1.0.0" directly in the "NODE_PATH" directory and one in the node_modules folder in the package-b module, the package-b module uses package-c and I would expect that the require (or import) would load the package-c file in the node_modules directory within package-b, instead esbuild resolves to the package-c module in the root of the "NODE_PATH" directory.

You can reproduce this bug with the linked repository:

How to reproduce:

# bundle project
> go run main.go
# go to ./project
> export NODE_PATH=$(pwd)/../NODE_PATH/node_modules
> node ./index.js
# output
# package_a: 1.0.0
# package_b: package_c version:2.0.0 (as expected, should be resolved to the version relative to the using module)
> node ./out.js 
# output
# package_a: 1.0.0
# package_b: package_c version:undefined (should be 2.0.0, but displayed is undefined and bundled is the "1.0.0" verison which ist in the root of NODE_PATH

node resolves the "package-c" dependency to the node_modules folder relative to the using module, esbuild resolves the "package-c" dependency to the module in the root of the NODE_PATH variable (just check the bundled out.js)

Maybe esbuild should mimic the resolution algorithm of nodejs?

Tested with esbuild 0.11.5 and node 14.15.1 on a windows PC

[evanw]

Thanks for the report, and especially for making it easy to reproduce. The reason why this works this way is that I implemented esbuild's path resolution by following node's own documentation: https://nodejs.org/api/modules.html#modules_all_together. This is the relevant part:

LOAD_NODE_MODULES(X, START)
1. let DIRS = NODE_MODULES_PATHS(START)
2. for each DIR in DIRS:
   a. LOAD_PACKAGE_EXPORTS(X, DIR)
   b. LOAD_AS_FILE(DIR/X)
   c. LOAD_AS_DIRECTORY(DIR/X)

NODE_MODULES_PATHS(START)
1. let PARTS = path split(START)
2. let I = count of PARTS - 1
3. let DIRS = [GLOBAL_FOLDERS]
4. while I >= 0,
   a. if PARTS[I] = "node_modules" CONTINUE
   b. DIR = path join(PARTS[0 .. I] + "node_modules")
   c. DIRS = DIRS + DIR
   d. let I = I - 1
5. return DIRS

So according to the documentation, the algorithm is for each DIR in [GLOBAL_FOLDERS] + ["node_modules" folders] so NODE_PATH comes first. However, according to node's internal code (node/lib/internal/modules/cjs/loader.js), the order is the other way around:

  const nodePath = isWindows ? process.env.NODE_PATH : safeGetenv('NODE_PATH');
...
  const paths = [path.resolve(prefixDir, 'lib', 'node')];
...
  if (nodePath) {
    ArrayPrototypeUnshiftApply(paths, ArrayPrototypeFilter(
      StringPrototypeSplit(nodePath, path.delimiter),
      Boolean
    ));
  }

  modulePaths = paths;
...
    let paths = modulePaths;
    if (parent?.paths?.length) {
      paths = ArrayPrototypeConcat(parent.paths, paths);
    }

In this case the path list is _nodeModulePaths().concat(nodePath.split(':')) so NODE_PATH comes last. I guess this means the algorithm documentation is incorrect?

In any case, fixing this in esbuild should be straightforward.

[mzeiher]

wow interesting, I did not read the documentation 😅, I also just looked at the nodejs implementation

thanks for also filling a nodejs bug :)

[bmeck]

Just to note, global folders and NODE_PATH (both have been deprecated for a very long long long time) are not supported by import in node at all.

[mzeiher]

@bmeck I know :) I really like the feature in esbuild though, in webpack we used the resolve.modules feature this can now be done with the NodePaths option for esbuild
weback

...
resolve: {
    modules: ['node_modules', path.resolve('other paths for dependencies'), ],
  }
...

the "other paths for dependencies" can now be injected via the NodePaths property.... maybe this would also be a nice additional feature... to specify additional resolve paths?

[tannerkrewson]

Interesting... this release 0.11.6 changed my bundle. I had a nodePaths: ['node_modules'] set on ESBuild, so that ESBuild would only search in root level node_modules. Now, ESBuild is preferring and including some dependencies from nested node_modules of npm linked dependencies, which is causing some issues. Is there any way to do that now?

In Webpack, we were doing a resolve.modules with an absolute path to root level node_modules to do this.

[evanw]

You can insert whatever custom path resolution logic you need with a plugin: https://esbuild.github.io/plugins/#resolve-callbacks.

[mzeiher]

@evanw does this mean I have to reeimplement the node resolution algorithm (with package.json parsing, browser, module resolvinge etc..) or is it also possible to just delegate the resolution with a new base path to the built-in resolver? If I return a path it is handeld as "resolved" and the built-in resolver does not kick in... maybe this would be a nice feature, delgate to the built-in resolver with a new path so that NODE_PATH does not need to be used for such scenarios?

[evanw]

does this mean I have to reeimplement the node resolution algorithm

Others have already done that work. For example there is https://www.npmjs.com/package/enhanced-resolve from Webpack.