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";

[ljharb]

The entire JS ecosystem works like this, and anything that doesn’t quickly becomes a pariah and nobody uses it. TS itself doesn’t follow semver, but i would expect the same semver compliance from any TS package.

i agree “exports” isn’t used much yet - it’s a breaking change to add it, and it’s pretty new. However, there’s just no programmatic way a linter can reliably know what’s “internal” in a third-party package, precisely because there’s no configuration format to declare that besides the exports field.

i hear you on the value of a rule that forces deep imports, or prohibits them. If such a rule existed, I’d encourage you to force deep imports, since in fact that is the cleaner and best practice, and it obviates the need for treeshaking to attempt to clean up sloppy over-importing. no-internal-modules, however, is not that rule.

[adidahiya]

Thanks for the response. Overall it sounds like we disagree about this rule option and I'll have to look elsewhere for a rule to satisfy my linting requirements (or build it myself). I do have a few follow up questions, though...

1. What is the point of the no-interal-modules rule, then, in your opinion? What situations is it actually useful for? (The example in the docs is a little too simple.)
2. How do you think TS package authors should architect their packages to achieve what you consider to be the way "the entire JS ecosystem works"? The standard way to publish a TS-authored NPM package is to include the whole outDir folder with its .js and .d.ts files, for runtime and build-time module imports respectively (also sourcemaps). Are you suggesting that once I publish one kind of folder layout of my package, I can never change that layout without breaking semver? What if I move one utility function from one submodule to another, deep within the folder layout? Even if that utility function is an implementation detail which package consumers shouldn't care about, that would still be considered breaking semver?
3. With regards to "there’s just no programmatic way a linter can reliably know what’s “internal” in a third-party package"... IMO we can at least make a useful (but obviously incomplete) assumption that packages without an "exports" field in their manifest want you to import from the root. Exceptions to this assumption would be flagged by the lint rule, but you could suppress those with inline disable flags. Again, in practice, this worked very well for us for years with TSLint's no-submodule-imports.

[ljharb]

I think a separate rule should exist - I'm willing to accept a PR for it to this plugin, though!

1. it's primarily for large monorepos, to prevent one project from importing "internal" pieces of another project.
2. Yes, in all packages, you can never change a folder layout (in a way that makes an existing require/import path break) without bumping the major version. This is best mitigated with exports, but can also be mitigated with npmignore/files so only needed things are published, or by bundling so that "internal" files don't exist in the package. Every single importable/requireable specifier is part of the API of your package. TS and JS are no different in this.
3. That would be an intensely incorrect assumption. I have hundreds of packages where i explicitly require you to deep-import, and simply don't provide everything via the main, because I find deep importing to be a best practice.

[teebszet]

it's primarily for large monorepos, to prevent one project from importing "internal" pieces of another project.

so I guess this rule was only meant to be useful with the "forbid" rule? seems like an arbitrary line to draw, given the default behaviour with this rule is to not allow deep imports from anywhere..

you've already added in "allow", which presumably was to make this rule more flexible, why not make the "allow" rule able to allow relative patterns?