Feature request: exclude devDependencies about import-js HOT 16 CLOSED

Just for some context: this would include reading the package.json file in the project root, look at devDependencies and exclude all subfolders inside the node_modules folder with names corresponding to a key in the devDependencies object.

This is definitely doable.

from import-js.

Yes, that's the idea.

from import-js.

I've been thinking about this for a while, and I don't know if it (always) makes sense to exclude devDependencies. What if you are working on a dev project? We could add a configuration option to turn it on/off, but I'm not sure that's good either. Do you want to elaborate some more on why this feature is important?

from import-js.

The idea here is similar to the reason we added the excludes option to prevent nested node_modules directories from being matched for import-js. Since the application doesn't explicitly depend on the module, it shouldn't be easy to import that JS into application code.

The more I think of this, however, I think instead of excluding devDependencies, we actually need to include only dependencies and peerDependencies for the node_modules directory. This is because shared dependencies can be deduped out into the top level and we don't want to import these packages unless the application explicitly depends on them. This will cover both cases.

from import-js.

I see, that makes sense. How about we squash together all dependencies and peerDependencies into an array of module names, then either feed that list into the find command or do some post-processing to filter out unwanted results. I like the former solution because it is more likely to become performant. Though we risk running into issues with the command becoming too long (though that's probably unlikely).

Both of these solutions assume that module names reflect folder names inside node_modules.

from import-js.

...they also assume that the node_modules folder is always named node_modules.

from import-js.

@lencioni I tried a quick implementation. I played around with it a bit in the Brigade repo, and I'm not sure i see the value of the extra filtering. I think it has the potential of making import-js feeling non-deterministic. The behavior is "correct", but a bit magical.

Can you try it out and see if you think it's worth completing? I need to add tests and perhaps clean up some code. But I want to make sure you try it out first.

from import-js.

I think I got that commit backwards...

from import-js.

I'll wait to try it out until the commit is forwards. Let me know when you'd like me to take a look.

from import-js.

Alright, I turned it around:
1660165

Try it out and let me know what you think!

from import-js.

This is great! In the Brigade codebase the most painful thing that I regularly import is Utils which goes from:

[import-js] Pick js module to import for 'Utils': (0.16s)
1: autoprefixer/lib/utils
2: center-align/utils
3: esutils/lib/utils
4: express/lib/utils
5: flux/utils
6: handlebars/dist/amd/handlebars/utils
7: handlebars/dist/cjs/handlebars/utils
8: handlebars/lib/handlebars/utils
9: hawk/lib/utils
10: hawk/test/utils
11: inquirer/lib/utils/utils
12: intl-messageformat/lib/utils
13: intl-messageformat/src/utils
14: jstransform/src/utils
15: qs/lib/utils
16: qs/test/utils
17: react-intl/lib/utils
18: react-intl/src/utils
19: support/utils
20: tether/src/js/utils
21: uglify-js/lib/utils
Type number and <Enter> or click with mouse (empty cancels):

to:

[import-js] Pick js module to import for 'Utils': (0.08s)
1: flux/utils
2: intl-messageformat/lib/utils
3: intl-messageformat/src/utils
4: react-intl/lib/utils
5: react-intl/src/utils
6: support/utils
7: tether/src/js/utils
Type number and <Enter> or click with mouse (empty cancels):

I think we should maybe even take this a step further and treat node_modules entirely differently--instead of adding each of the dependencies and peerDependencies directories to the list of directories to find files in, we should just treat these as top-level things that can be imported. That would cut down this list from 7 to 1, which would be really nice. Plus, most of the time you are importing the package itself which uses main. For the special cases where you need something deeper, you could add some configuration to .importjs.json. What do you think?

from import-js.

Or if not for every dependency, perhaps just the ones with a main? You could probably combine it with some of the code from 716bfda.

from import-js.

That's a good idea, I like it. If I have time today or tomorrow, I can try it out. You are also welcome to give it a shot if you like.

I'm a little bit worried that reducing the list of matches can lead to people not trusting the plugin, or think that it's broken in some way (smarter node_modules matching will make the tool a little bit more "magic"). But I think it's fine in this case, because we can still allow people to put node_modules in lookup_paths, and then get the old find-all behavior. But if it's left out (which we can recommend in the README), we have a much more efficient importer in place.

from import-js.

Alright, let me know what you think of a15a482

I tried it out quickly, and I think it might be exactly what we need! Even though I didn't focus on performance, simply avoiding to run the find command inside node_modules helps a_lot.

Make sure to modify .import-js.json and remove node_modules from lookup_paths before you try.

Oh, and Utils?

[import-js] Imported `support/utils` (0.07s)

😃

from import-js.

I just tried it out and it seems to work perfectly. This is a solid improvement.

from import-js.

Thanks! I cleaned it up a bit and squashed the WIPs into a single commit pushed to a new branch. I'll remove the WIP branch eventually.

from import-js.