google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.31k stars 1.15k forks source link

NODE_PATH is not respected when resolving package.json for ES6 module. #3148

Open zavr-1 opened 5 years ago

zavr-1 commented 5 years ago

I want to stub internal Node.js modules such as stream and I can do it if I place the mocks in the node_modules folder inside of my package that I compile, however I don't want to do that and I want to pass the specific location for the package.json mock.

java -jar /Users/zavr/adc/depack/node_modules/google-closure-compiler-java/compiler.jar 
  --compilation_level=ADVANCED 
  --language_in=ECMASCRIPT_2018 
  --language_out=ECMASCRIPT_2017 
  --module_resolution=NODE 
  --formatting=PRETTY_PRINT 
  --externs=externs/stream.js
   --externs=externs/Buffer.js 
  --js=interns/node_modules/stream/package.json 
  --js=interns/node_modules/stream/index.js 
  --js=example/example2.js 
  --process_common_js_modules=true

This, unfortunately, does not work because example/example2.js does not have interns/node_modules/stream/package.json in the NODE_PATH when resolving. To solve this, I'm setting spawnOptions on the compiler with the env property set to include the NODE_PATH:

spawnOptions = {
    env: {
        NODE_PATH: '/Users/zavr/adc/depack/interns',
    },
}

However, this also results in an error:

example/example2.js:2: ERROR - Failed to load module "stream"
import { Readable } from 'stream'

When I try to create node_modules in the level above, I also get the error:

java --compilation_level=ADVANCED 
  --language_in=ECMASCRIPT_2018 
  --language_out=ECMASCRIPT_2017 
  --module_resolution=NODE 
  --formatting=PRETTY_PRINT 
  --externs=externs/stream.js 
  --externs=externs/Buffer.js 
  --js=../node_modules/stream/package.json 
  --js=../node_modules/stream/index.js 
  --js=example/example2.js 
  --process_common_js_modules=true

cat ../node_modules/stream/package.json:

{
  "name": "stream",
  "main": "index.js"
}

cat ../node_modules/stream/index.js:

module.exports = DEPACK$stream // eslint-disable-line

Is it possible to get the stream package resolved to something that is not inside of node_modules folder of the project directory from which I run Closure?

brad4d commented 5 years ago

@ChadKillingsworth is this something you would want, also?

brad4d commented 5 years ago

Created internal Google issue b/119841270

ChadKillingsworth commented 5 years ago

This really seems like the wrong way to solve the problem. I'm not sure we want to influence module resolution via environment variables.

Bundlers like webpack replace these references before compilation.

The other option is to support aliasing packages.

zavr-1 commented 5 years ago

@ChadKillingsworth the module resolution is not the only problem because the correct module resolution would also look up for files in the parent directory of the given module, i.e., ~/.. And NODE_PATH is only part of the fact that the module resolution is implemented not correctly.

ChadKillingsworth commented 5 years ago

Closure-compiler should already locate modules where the node_modules folder is in a parent directory. As long as those source files were provided to the compilation.

Do you have a repo case that demonstrated this not working? If so - please open a separate issue with it.

zavr-1 commented 5 years ago

@ChadKillingsworth Hi Chad by the looks of the code and according to what you're saying, you misinterpret the resolution algorithm. If I run have a project a in ~, then ~/node_modules will also be scanned by Node.js. If I am wrong, then it still does not work and I will make a repo.

ChadKillingsworth commented 5 years ago

The compiler scans nothing. You have to provide it all potential source files. But once they are provided, the compiler will walk up the directory tree looking for node_modules folders at each step.

zavr-1 commented 5 years ago

@ChadKillingsworth You don't understand, the node resolution system is such is that it will try to load the module from ~/node_modules for ~/abc project, and then ~/../node_modules etc it does not care whether node_modules is in the path or not. Even if I pass the path to these packages as you suggest, the compiler will fail which the bug report is about. Everything is because the algorithm is implemented incorrectly.

nbeloglazov commented 5 years ago

If I understand correctly the problem is the following: given the following structure:

project (from this folder closure compiler is called)
|--node_modules (1)
|  |--...
|  +--...
|
|--example
|  +--example.js
|
+--interns
   +--node_modules (2)
      +--stream
         |--package.json
         +--index.js

and example.js has require('stream') you want to to tell closure compiler that it should use node_modules inside interns folder when resolving modules in example.js. And for some reason you can't put streams folder inside node_modules (1). I had similar problem when setting up closure compiler typechecking on nodejs at Google as we use bazel which doesn't allow you create or move files to the root of the project.

I ended up using --js_module_root flag as a workaround. So in your case it would looks the following:

java -jar /Users/zavr/adc/depack/node_modules/google-closure-compiler-java/compiler.jar 
  --compilation_level=ADVANCED 
  --language_in=ECMASCRIPT_2018 
  --language_out=ECMASCRIPT_2017 
  --module_resolution=NODE 
  --formatting=PRETTY_PRINT 
  --externs=externs/stream.js
  --externs=externs/Buffer.js 
  --js=interns/node_modules/stream/package.json 
  --js=interns/node_modules/stream/index.js 
  --js=example/example2.js 
  --js_module_root=interns
  --process_common_js_modules=true

When closure compiler runs it will strip "interns" prefix from provided paths and will treat interns/node_modules/stream as node_modules/stream which makes it a "root" for the example folder. See this test.

Personally I would be happy if compiler had a way to pass a map of modules => paths like:

{
  'stream': 'path/to/folder/stream.js',
  'fs': 'path/to/other/folder/fs.js',
}

It would make life easier in environments where it's hard to assemble all "root" modules in the root directory.

ChadKillingsworth commented 5 years ago

Unfortunately a map like that does support more complex use cases where multiple versions of the same module are present and require nested node_modules folders. The lookup algorithm is influenced by the path of the host file.

Your example would only support simplistic cases. However, that general idea is being discussed for browsers: https://github.com/WICG/import-maps

Webpack solves this with a numeric map - however it also influences the import statement.

Utilizing the --json_streams flag to load sources, you can specify any path you want for any file. This allows you to fully control paths and do the type of manipulation desired. However, it will of course have an impact on source maps - but embedding sources content in the map can solve that problem.

nbeloglazov commented 5 years ago

Wasn't aware about import-maps. Yes, that would be very useful for my usecase. And about --json_streams - this parameter allows to specify input files via stdin vs reading from filesystem? And with it all files must be passed through stdin or it falls back to filesystem when file is missing from stdin?