node resolver: respect NODE_PATH

[benmosher]

Automagically split (on path.delimiter), path.resolve relative to CWD, and add to paths.

(inspired by #130)

[gnapse]

Hi. Is this being worked on?

[ljharb]

I'd prefer we not support NODE_PATH - it's deprecated, and node won't be implementing it for ES Modules.

[ryan-roemer]

As far as I know (and reading from the docs) NODE_PATH is not deprecated. Moreover, it is part of the spec and there are many tools that have relied on it for years for require().

From https://nodejs.org/api/modules.html#modules_loading_from_the_global_folders

NODE_PATH is still supported, but is less necessary now that the Node.js ecosystem has settled on a convention for locating dependent modules. Sometimes deployments that rely on NODE_PATH show surprising behavior when people are unaware that NODE_PATH must be set. Sometimes a module's dependencies change, causing a different version (or even a different module) to be loaded as the NODE_PATH is searched.

Doing a overly-simplistic search on NODE_PATH usage in JS code like https://github.com/search?l=JavaScript&q=NODE_PATH&type=Code&utf8=%E2%9C%93 yields a lot of hits.

Please support this!

[ljharb]

At the very least, it's highly discouraged in the community to use it - and as I said, node has explicitly indicated that they will never implement anything like it for native ES Modules, so in the fullness of time, you'll effectively be forced off of the pattern anyways.

[mockdeep]

@ljharb do you have a link to where future support for NODE_PATH is discussed? It seems pretty unlikely that they would drop support for something that is so heavily used.

[gnapse]

Apps created with create-react-app use NODE_PATH (see here). I don't know where the idea came from that it is deprecated.

[ljharb]

@mockdeep support won't be dropped since ESM is new. It just won't be added, because the only folder you should be importing from is node_modules.

Just because something is used, even in CRA, doesn't make it a good idea ¯\_(ツ)_/¯

[ryan-roemer]

I for one would vote for "when resolving require()'s, use the heuristic that Node uses" (aka include NODE_MODULES). Just the way our code and applications do.

Similarly, when ESM lands, I would vote for "when resolving import's, use the heuristic that Node uses" (aka don't add NODE_MODULES support there).

[gnapse]

Ok, so let me try and understand something, because I imagine there must be a sane way to do it:

In a sizeable react application, a common way to organize it is to have a src/components/ folder where you have most of your publicly available components to be used in the app, but that are still implemented by the app itself, not available through an external package dependency.

Then in the pages you usually want to have access to many of these components, and it's a bit of a hassle to have to type a relative path to all your app-specific components in order to import them in the context you want them.

Is there a way to achieve doing things like this below?

import MyComponent from 'components/MyComponent';

instead of having to do something like this...

import MyComponent from '../../../../components/MyComponent';

...and be able to make this eslint plugin pick that up and realize that there's no missing components package?

[ljharb]

@ryan-roemer without a doubt, if this module does support NODE_PATH, then what you're describing is precisely how it would be done.

@gnapse No, you should be using the dots. If you don't like the dots, extract things to a separate npm-installed package. alextes/javascript-best-practice#1 (comment) may be helpful/relevant.

[gnapse]

@ljharb I have an alternative solution that the issue you link to does not cover. It's ember-cli's approach, where your app is considered implicitly as a package. Therefore you can import things like:

import MyComponent from 'my-app/components/MyComponent';

This is referring to /src/components/MyComponent within you're own my-app folder structure. my-app is the package name of your very own app, as declared in its ./package.json file.

Would this be considered bad practice too? If so, why?

[ljharb]

@gnapse I'm not sure how ember does "considered implicitly as a package", but if it does it using one of the techniques in that link, then the mentioned caveats apply - if not, then that's a very bad practice, yes.

[mockdeep]

@ljharb you didn't answer my question. Is there a conversation you can point us to that discusses how node is planning to leave off supporting NODE_PATH when implementing es6 imports?

Just because something is used, even in CRA, doesn't make it a good idea ¯_(ツ)_/¯

This is a non-argument. It doesn't make it a bad idea either.

[ljharb]

@mockdeep I agree; I was responding to a non-argument ("it's used in CRA") with an equally valid non-argument.

As to your question: the official node module proposal indicates this - it's the concrete plan of record, and is definitely not going to change.

[mockdeep]

definitely not going to change

You use such strong wording. Isn't a draft something that is by definition subject to change?

[ljharb]

@mockdeep I'm using the same strength as the author of that draft; neither they nor anyone I'm aware of on the node TSC wants that specific aspect of the draft to change.

[gnapse]

Referring to stuff by relative paths is not always a good idea. I tend to organize my apps as modularly as possible. Within what I consider a module (a self-contained feature arranged in a set of files within a directory) I use relative paths because everything within that module is tightly coupled. But when crossing boundaries across modules (e.g. one module importing another module) I dislike using the relative path approach to import the other module's index.js file (which is the only public interface of that other module). I dislike it because it tightly couples one module to the relative location of the other. If module A imports module B, then module A is tightly coupled to module's B location. If for whatever reason I move module A around, I have to also change all its imports of things outside of it.

There you have an explanation of the logic behind the feature request. Can you explain me what is wrong with considering your own app as a package, alongside all the other packages in node_modules? What's the package name in package.json for? What's inherently wrong about Ember's approach which is to allow to import from your own app's directory structure as if your app was a package itself?

I agree that other things, like wanting to put several other folders in NODE_PATH and magically import from within them is wrong. I came into this discussion not thinking that, and I've been convinced. But I still think there's a need to import from your own app's namespace without having to use relative paths all the time.

[benmosher]

Ignoring for a moment whether or not using NODE_PATH at application runtime is a "good" or "bad" idea: you can configure the Node resolver explicitly via your .eslintrc with search locations. See the readme: https://www.npmjs.com/package/eslint-import-resolver-node

from the example there:

# .eslintrc.yaml
settings:
  import/resolver:
    node: 
      paths:
        # an array of absolute paths which will also be searched 
        # think NODE_PATH 
        - /usr/local/share/global_modules

If you use .eslintrc.js, you could use something like path.resolve(__dirname, "path/relative/to/.eslintrc.js") if you want "relative" paths.

This issue was more about whether the Node resolver should automatically check NODE_PATH for modules at lint time, which is actually probably a confusing and terrible idea for its own reasons because anything on that path could now be required as a lint dependency.

Given that, I'll close this issue and recommend that you encode your runtime NODE_PATH into the relevant .eslintrc.

[ryan-roemer]

@benmosher -- Thanks for taking the time to review this issue.

Having NODE_PATH isn't going to bring in extra require's, just give an opportunity for existing ones to search. And having NODE_PATH as an environment variable, not a statically coded path is what a lot of our tooling / sometimes app uses.

In particular, our multi-repo wrangling tool lank offers an alternative to the "doesn't work right in many cases" npm link for working on interrelated modules. It achieves this by "punching holes" (deleting all related modules in the node_modules tree) and using NODE_PATH to stitch these repos back together.

A totally weird and nonstandard way of doing things, but also one completely supported by the current require spec.

I've been having to one-off disable things like:

const foo = require("foo"); // eslint-disable-line import/no-unresolved

I encourage you to reconsider and just support the spec, so that when this plugin says it has a "Node resolver", that resolver actually resolves like Node does. Thanks!

As an alternative, do you have an example that would work instead with the actual NODE_PATH environment variable?

[ryan-roemer]

Or, putting it a different way, there's no downside to doing NODE_PATH resolution -- the application or library code that this plugin is attesting to is actually going to do that resolution at runtime. A separate way to think about it is this plugin would do something different than what would really happen at runtime if it doesn't support NODE_PATH.

At the end of the day, I don't think all of us have to agree on whether or not NODE_PATH is a good thing or not for require. I just humbly offer we should support the require resolution like Node does so that this plugin behaves just like the real code does.

[benmosher]

I'd disagree: the lint-time NODE_PATH should be specifying a search path for the linter to find code it should be running for linting. Unless you're ESLint, or this plugin, you're not using the code you're linting so its NODE_PATH should/could be different.

I agree that 99% of the time no one is using it, and 99% of the remaining 1% it would work fine. But it's one more thing to maintain and given that I suspect it's generally not needed, I don't think it's worth the investment. (sorry. 😅)

Fortunately, if you want to use the lint-time NODE_PATH, you could us a JS .eslintrc and do something like

// .eslintrc.js
const path = require('path')  // assuming you use cwd-relative paths in your NODE_PATH
exports.settings = {
  "import/resolver": {
    node: { paths: process.env.NODE_PATH.split(":").map(path.resolve) }
  }
}

and I think that will give you the behavior you're looking for.

[ryan-roemer]

@benmosher -- All our eslint configs are YAML. And chance there's a YAML version of that?

[benmosher]

No way to use functions in YAML config AFAIK. But it's easy enough to convert. (I prefer YAML as well, but it is handy to be able to use the power afforded by JS config when needed)

[artokun]

After a bunch of research to try to get NODE_PATH = src/ to work with CRA, this is the .eslintrc file I got to work. @benmosher js version I couldn't get to work. You might just need to hardcode the moduleDirectory to fit your needs, though if you are using CRA the following should work.

JSON

{
  "parser": "babel-eslint",
  "env": {
    "browser": "true",
    "node": "true",
    "jest": "true"
  },
  "extends": "airbnb",
  "settings": {
    "import/resolver": {
      "node": {
        "moduleDirectory": ["node_modules", "src/"]
      }
    }
  }
}

YAML

---
parser: babel-eslint
env:
  browser: 'true'
  node: 'true'
  jest: 'true'
extends: airbnb
settings:
  import/resolver:
    node:
      moduleDirectory:
      - node_modules
      - src/