Only apply no-internal-modules to node_modules

[felixfbecker]

I'd like to forbid deep internal imports from libraries, e.g.

import internalFunction from 'some-lib/deep/down/in/the/dark/internalFunction'

but allow importing from anywhere in the current project (i.e. any relative imports):

import whatever from '../../../some/project/folder/module'
import whatever from './app/some/module'

just like TSLint's no-submodule-imports

Currently, the last example always fails.

I tried to specify allow: ['./**/*', '../**/*'] but that doesn't make a difference.

Could there be an option added for this, e.g. nodeModulesOnly, or ignoreRelativeImports?

[ljharb]

Why?? Deep imports are best for many reasons, including making treeshaking unnecessary. Many packages require you to deep import, for this reason.

[felixfbecker]

JavaScript doesn't have "private" modules like other languages, so most npm packages have a single index.js that (re)exports all the public API and every other module is considered internal.

There are some exceptions of course, packages with well-documented secondary entry points. We maintain a list of these in the allow array, e.g. rxjs/operators is allowed. But it's actually not many, at least not from those we use.

The motivation for this comes from experience of having people import internal functions from packages (open source as well as private), then the package maintainer making breaking changes to these because they were not aware that code relied on them and they were never intended to be imported. Especially for private packages, this is to ensure people define index.js files that clearly define the public API instead of creating coupling to internals.

I haven't noticed a package where webpack was not able to follow the reexports from index.js.

What other reasons are there? Especially if you import multiple things from the same package, named imports are much more compact than deep imports. E.g. Rx switched to a single operators.js entry point for this reason in v6:

import { map, filter, switchMap, take, startWith, takeUntil } from 'rxjs/operators'
// vs
import map from 'rxjs/operators/map'
import filter from 'rxjs/operators/filter'
import switchMap from 'rxjs/operators/switchMap'
import take from 'rxjs/operators/take'
import startWith from 'rxjs/operators/startWith'
import takeUntil from 'rxjs/operators/takeUntil'

[ljharb]

That's because nothing reachable is private; however node is adding support for an "exports" field in package.json for both CJS and ESM that would actually forbid importing - in which case a linter rule wouldn't have any value.

Webpack has chosen not to implement (the equally trivial and achievable) CJS treeshaking, so there are plenty of packages where it can't "follow" the re-exports.

[felixfbecker]

Well for the packages we use it's either

• Small packages that only export a single function from their index.js
• Bigger libraries that are shipped with ESM (where tree shaking works great)

exports field sounds interesting but I don't see all of our packages and tools support that anytime soon. Meanwhile the problem of importing internal things is a real problem we've had and currently solve with the TSLint rule. However, TSLint is deprecated so we're trying to switch to ESLint, and this is one of the few rules that don't have a full equivalent yet.

If you take the stance "That's because nothing reachable is private" that would mean the only way to have internals is to put the entire library in a single file, which is not feasible for even medium-sized libraries. From my experience the convention is more like "Everything documented is public, everything undocumented is private". I.e. you should import things how the docs tell you to import them, and if you import some file in/a/deep/internal/undocumented/folder there is no guarantee that will still work the next version.

[ljharb]

Yes, I do take that stance, which means that "internals" are just part of the public API.

I feel that this is a better fit for a custom rule, since it encourages a bad practice.

[felixfbecker]

Whether it is a good or bad practice is subjective, but it definitely is a common practice. Some say named exports are a bad practice, some say default exports are a bad practice. eslint-plugin-import has rules to enforce use of either 🤷‍♂

Maybe we can leave this open in case there are more people have a need for this.

[adjerbetian]

I would also like to have such a possibility :

• if a folder has an index.js/.ts file, I would like to prevent access to whatever is in this folder (but the index file of course 😅)
• if there is no index file, then everything is public

[ljharb]

@adjerbetian you can't impose a linter rule on your package's consumers, so you can't actually prevent anything.

The upcoming "exports" field is the only way, without bundling with a tool like rollup, you can actually prevent access to "internal" things.

[adjerbetian]

@ljharb Yeah, you're right about that. My stand is more that if the library authors created an index file in a folder, it's probably because they don't want to export what is not in the index file. But indeed, it's a hypothesis that might not always be true, after all, people do what they want 😅.

And indeed, the "exports" field you mention feels like a good idea when it will come 👍.

Anyway, if there is a way to have such an extra rule here, I would appreciate it. If not, well, too bad, but it won't hurt so much. It's already very cool to have the no-internal-modules rule locally 🎉 😉.

[felixfbecker]

@ljharb no, you can't impose a linter rule on your package's consumers, so you can't actually prevent anything as a package creator. But I don't see how that is an argument to not even offer such a lint rule to package consumers. It is the same as with many things in JavaScript, e.g. underscored-prefixed properties being considered private. And you can say "proposed private fields are the only way" and that is correct, but it's a fact that conventions already exist that are widely used and will be for a long time. It is a known convention, you can document it as package creator, and as a package consumer internally in a company or in your own projects, you can enforce it with lint rule. Clearly it is advantageous to prevent contributors from relying on internals that could change at any patch version - be it underscore-prefixed properties, or files nested deeply in a package's file hierarchy. Both are considered internals by convention in many packages.

[ljharb]

Having an index file is a common pattern, to be sure, but i highly disagree that it’s common to ask consumers to only import via the index. It’s a convenience (one with the extra burden of needing treeshaking), not a better mechanism.

Separately, there’s a reason the Airbnb guide forbids underscored properties - convention is irrelevant, if you change one and break someone, you broke them. npm broke node one time doing that - this isn’t just pedantry.

[adidahiya]

Even though the proposed change to this lint rule will never strongly enforce that modules are only imported from their index.js entrypoint, I think it is still useful to have. We used the same semantics in the TSLint rule no-submodule-imports for years successfully. At the very least it signals intent to developers and flags when they may be about to use an unsupported API. Very few packages I use require deep imports. I'd like to make them the exception to the rule in our coding standards.

The "exports" field may be the only way to truly prevent access to internal things. Fine. But I want to strongly suggest that using a lint rule instead. The reasoning is similar to why some developers choose to enforce coding guidelines via lint rules rather than via the TypeScript compiler (which enforces stronger checks).

Would you accept a PR for this proposed rule option?

[ljharb]

Lint rules apply to your own code. Why would you care what you're importing from third-party code? If it's importable, it's part of the API, and it can't be broken without a major bump or a semver violation. If you control the code, you'd use "exports".

[adidahiya]

Lint rules apply to your own code. Why would you care what you're importing from third-party code?

Import statements are my own code. They declare my code's dependencies on third party code.

If it's importable, it's part of the API, and it can't be broken without a major bump or a semver violation.

In theory, this sounds like a nice idea. In practice, though, much of the JS ecosystem (especially the TS ecosystem) does not work like this, in my experience. As shown by the Node.js package.json package entry points documentation, clearly there is a desire for package authors to encapsulate their code and only declare certain parts of the API as public. However, most packages on NPM do not use this feature (the "exports" field) in their manifest yet.

Many packages ship with all their source code laid out in the same folder layout in which they were authored in their NPM artifact, including all kinds of implementation details. Package authors (including myself) sometimes wish to refactor those implementation details without making a major version bump due to semver. I could go around telling all the authors of all my dependencies to add the "exports" field and be explicit about their package API, but in the meantime, a lint rule to mitigate potential submodule import issues is sorely needed.

And then there are other exceptions to semver in the NPM ecosystem, where breaking changes are inadvertently introduced (nobody's perfect), or packages simply choose to not follow standard semver for good reason (for example, the typescript package).

Lastly, unrelated to the above arguments, submodule imports are sometimes just plain messy/crufty. Consider the case of an editor plugin which auto-imports for you and incorrectly introduces a deep import in place of an index.js import. Here I am assuming that I have set up my bundler to use ES modules and tree-shake effectively. This is exactly the kind of code lint which a linter should be able to fix:

import { symbolExportedFromIndex } from "some-lib/with/a/deep/import";
// should be instead:
import { symbolExportedFromIndex } from "some-lib";