Match on files within top-level depedencies

[lencioni]

Folks may want to import files from within packages they depend on. I think we need to include these files, or a subset of these files in the files that we match on. This may end up causing relevance issues, which we might be able to resolve with better ranking, with a stricter opt-in mechanism (see below), or both.

In #340 (comment) I mentioned:

One idea I have is to only consider files in the directories.lib directory specified in the package's package.json file. This would allow packages to opt in to this behavior which would help us avoid matching on random example files, documentation, test files, and other private modules.

I think we could start by adding this feature and allowing packages to opt in to having the files in their directories.lib directory be matched. We can always expand it later if necessary.

I think we need to do this for 2.0, since we removed lookupPaths.

[lencioni]

I think it would be good to write some unit tests for findMatchingFiles.js and findJsModulesFor.js before adding this feature.

[mike623]

So, for now. may I conclude that there is no way to import sample from 'lodash/sample' or import Link from 'react-router/lib/Link' right? I can add it using aliases one by one but it is inefficient

[trotzig]

Unfortunately, I think that's the case. I forgot to check back to this issue before releasing 2.0, so no fix was included.

For lodash, you could depend on individual functions instead. npm install --save lodash-sample (and others) instead of npm install --save lodash. If you then combine it with ignorePackagePrefixes: ['lodash.'] in import-js config, you'll end up importing the right thing.

For react-router/lib/Link I can't think of a better workaround than adding an alias.

[mike623]

thx for help. Will wait for improvement

[trotzig]

@lencioni: do you still think this is an issue? Most packages I've come across export everything in the main file, and now that we have support for finding named exports from package dependencies it might not be an issue anymore. Having to reach in to packages like this seems like an anti-pattern. For instance, the react-router Link is exported through the main module. So you can do

import { Link } from 'react-router';
<Link/>

[lencioni]

@trotzig I think this is going to be useful for a bunch of packages still, particularly component bundle packages like material-ui. Once tree-shaking becomes more mainstream it might not matter anymore, but until then I'd still expect a lot of packages to recommend importing files within the packages directly to avoid bringing in everything and bloating bundle sizes.

[trotzig]

By "bloating bundle sizes", I take it you mean webpack bundles for instance? That's a valid concern. It's a little tricky. I do think it's an anti-pattern, but one that's absolutely necessary in some cases.

If you can think of ways to solve this within import-js, I'm all ears. The naive solution would be to reach in to every package dependency in search for modules and their exports. That's potentially expensive, so I've been avoiding that.

In the meantime, I do think using aliases is a good-enough workaround. I expect projects to have a handful of these at most, at which point adding them to aliases isn't terrible. Let me know if that assumption is wrong though, there might be use-cases I'm not seeing.

[lencioni]

By "bloating bundle sizes", I take it you mean webpack bundles for instance?

Yeah, or browserify bundles or whatever build tool you are using that doesn't do tree-shaking. If you used the named import from the main package without tree-shaking, you end up bringing in the entire package.

I expect projects to have a handful of these at most, at which point adding them to aliases isn't terrible.

As one data point, material-ui seems to have at least 41 different "top-level" components that could be imported (I counted the directories starting with uppercase letters here https://github.com/callemall/material-ui/tree/master/src). We have an internal package that is structured like this that currently has 105, but it is changing all the time. Aliases aren't a great solution for us here.

If you can think of ways to solve this within import-js, I'm all ears. The naive solution would be to reach in to every package dependency in search for modules and their exports. That's potentially expensive, so I've been avoiding that.

What do you think about my idea in the original post?

I think we could start by adding this feature and allowing packages to opt in to having the files in their directories.lib directory be matched.

Alternatively, we could add a configuration option to import-js that would point import-js at specific packages or even directories within packages to index and match on.

[trotzig]

I think we could start by adding this feature and allowing packages to opt in to having the files in their directories.lib directory be matched.

Ah, sorry. Forgot about this. Yes, I like it. The directories.lib setting isn't something I see a lot of however. Material-ui doesn't seem to have it specified. https://github.com/callemall/material-ui/blob/master/package.json

Alternatively, we could add a configuration option to import-js that would point import-js at specific packages or even directories within packages to index and match on.

I like this too. Something like

  allowInternalModulesFor: ['react-router', 'material-ui'],

I'm using "internal modules" here to match the eslint rule that disallows importing this way: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-internal-modules.md

[lencioni]

Yeah I don't think I've ever seen it actually used. But, using it might give people a reason to add it.

[lencioni]

As for allowInternalModulesFor, it might be important to specify specific sub-trees to look in, e.g. a build directory or something like that.

[trotzig]

What if instead of allowInternalModulesFor, we had a includes? It overwrites whatever is defined in excludes, and then you can do things like:

excludes: [
  './**/build/**',
],
includes: [
  './node_modules/material-ui/lib/**', 
],

Since node_modules are excluded by default, it's a little hard to understand I guess. But I like being able to specify glob patterns. I don't know how hard this would be to implement though. The current setup around indexing package dependencies is a little brittle, and we might have to refactor a few things before we add this. What's mostly complicated is that we "need" to support non-watchman setups, where listening to the node_modules folder is currently very expensive.

[lencioni]

includes is interesting, but how do you know which has precedence, includes or excludes? Would it be whichever has higher specificity?

[trotzig]

Yeah, that would be an issue. I guess the naive answer is to give includes precedence, and document that in the README. I could see this adding confusion though, and users will likely add includes without needing to.

[ljharb]

Same as gitignore - includes always grabs filenames first, excludes always subtracts from that list.

[lencioni]

I've given this some more thought. Two things come to mind:

1. If we fast forward a year or two, as tree-shaking becomes more prevalent, I would expect to see more people using named imports/exports rather than deep imports.
2. It is unlikely that you will want to allow importing of any file from any top-level dependency--usually just a subset of the files, perhaps from a specific directory.

Given these two pieces of information, it seems that the right solution would be something simple. I think that adding an includes option (or something like it) to config is the way to go on this.

Some open questions:

Should we scope the option to packages? e.g.

packageIncludes: [
  'my-package/foo/**',
]

or should the option be more general? e.g.

includes: [
  './node_modules/my-package/foo/**',
],

If we keep it general, should we allow package includes to be specified without the ./node_modules/ part? Also if we keep it general, should we give it a default value of something like ['./**'] (the implication here being that if someone specifies includes in their configuration they would need to include the default if they want to keep it): includes: ['./**', './node_modules/my-package/foo/**'].

[ljharb]

It's a code smell if "node_modules" has to appear anywhere. If you want to support non-packages, I'd suggest two separate lists.

I think the default case is that you'll want to be able to import any file in a direct dep - the glob approach is nice, but I'd want my configuration to be "always include every file in every direct dependency". How is that achieved with your suggestion, noting that node_modules/*/** isn't ideal for a number of reasons?

[lencioni]

It's a code smell if "node_modules" has to appear anywhere.

Yes, I agree with this. I think the patterns in this config should understand foo/bar as ./node_modules/foo/bar.

If you want to support non-packages, I'd suggest two separate lists.

I'm not sure we need to support non-packages, but even if we did we could pretty easily achieve it in one list I think.

I think the default case is that you'll want to be able to import any file in a direct dep

I'm not convinced about this. How often are folks importing deep files from their dependencies? My hunch is that most dependencies are imported for the main or named exports (which will likely increase over time) and only a handful are intended to allow for importing nested files. And of those, usually the nested files that one may want to import on a regular basis are scoped to a directory or a subtree and not any file in the entire package.

The problem with making this look at all of these files by default is it can make the importing process unnecessarily noisy and frictioned since you are much more likely to end up with matches on files that you don't care about. For example, let's say I have a dependency on moment and I go to import moment. If we always include every file in every direct dependency for matching, you would be presented with at least these options:

import moment from 'moment';
import moment from 'moment/moment';
import moment from 'moment/src/moment';
import moment from 'moment/src/lib/moment/moment';

We could probably try to be smart and remove anything that is also main in the package.json but this list would still be unnecessarily confusing for folks. It seems better to have people opt in to this for specific glob patterns where they want this behavior.

How is that achieved with your suggestion, noting that node_modules/*/** isn't ideal for a number of reasons?

On the flipside, let's say we included all files in direct dependencies by default. How would I exclude all files except for a specific directory from a package? e.g. I only want to include foo/build/*/** but not foo/src/*/** or foo/tests/*/** or anything else that might get added.

[ljharb]

Every single person using lodash 5 and up will be importing deep files from it, for example, and not from within any directory. react-dates also has a number of things that are only accessible via deep imports.

Certainly you'd want to filter out everything that's not in "main". Another approach could be, only show things that aren't required elsewhere, but that'd filter out real entry points too.

As for src, I can see how that'd get tricky when it's included in the package - but I'd still want those options to show up.

I guess I'd expect the default to be "include all entrypoints from npm-installed modules", with the option to flip that, and then either an "exclude" or an "include" override list respectively, that was a hash keyed by module name, and with a value that's a list of patterns.

[hathawsh]

Until this issue is solved, here's a workaround for material-ui imports. I added this option to my .importjs.js:

    importStatementFormatter: (options) => {
      // Adjust material-ui imports to avoid accidentally importing the entire
      // material-ui bundle.
      let stmt = options.importStatement;
      const match = (
        /import \{\s*([A-Za-z,\s]+)\s*\} from '@material-ui\/core';/.exec(stmt));
      if (match) {
        const names = match[1].split(',');
        stmt = names.map((nameWithWhitespace) => {
          const name = nameWithWhitespace.trim();
          return `import ${name} from '@material-ui/core/${name}';`;
        }).join('\n');
      }
      return stmt;
    },

It seems to do the trick.