Galooshi / import-js

A tool to simplify importing JS modules
MIT License
525 stars 55 forks source link

Assumption is made that packageDependencies are node modules #319

Open rhettlivingston opened 8 years ago

rhettlivingston commented 8 years ago

There is at least one instance in the code (in findJsModulesFor) where an assumption is being made that the packages found in packageDependencies are all node_modules. This is no longer true. Some may be meteor packages.

I'm guessing that the above-noted instance might impact the functionality of goto? Are there other places making similar assumptions?

Do we need to add a routine to the environments that returns a path for a package identified by the environment and perhaps some of the other assumed values?

trotzig commented 8 years ago

It might be possible to avoid the direct dependency on the node_modules folder by using require.resolve (through our wrapper module requireResolve) instead of constructing that path to the package.json ourselves.

rhettlivingston commented 8 years ago

I think that we're starting to go through the same evolution that benmosher's eslint-import-plugin went through (spoken of here). I have noted that he has a well-defined and simple interface to those resolvers, that the two he offers (node and webpack) are published separately from his overall project, and that others are being published by third parties (more than he has listed).

There is already at least one case where the stock require.resolve doesn't work, Meteor absolute paths. Truthfully, there are others that you guys are trying to work around using aliases - probably mostly introduced by webpack as happened in Mr. Mosher's case.

If we were to utilize eslint-import-resolver-webpack (perhaps keyed by and implemented by the inclusion of a webpack environment) to attempt resolution of webpack module-names to a file, would not some of your current needs for aliases go away?

Basically, I'm proposing a) let's have some default resolution, but allow environments to offer special resolution routines and b) we might get some mileage out of the resolver packages used by eslint-plugin-import in implementing some of those environment specific resolution routines.

trotzig commented 8 years ago

If we were to utilize eslint-import-resolver-webpack (perhaps keyed by and implemented by the inclusion of a webpack environment) to attempt resolution of webpack module-names to a file, would not some of your current needs for aliases go away?

Maybe, though I'm not sure. There's one where we specify the loader at import-time:

  "aliases": {
    "branch": "imports?define=>false!branch-sdk"
  }

We could probably move this to webpack.config.js, but I quite like how it's clear that the import is non-standard just by looking at it. This one is an edge-case however, and I think the overall plan of extracting path resolving for packages sounds good. I'd prefer to do it in steps however, and start with a configuration method that we can implement for specific environments.

lencioni commented 8 years ago

@trotzig I think that weird import path will cause problems with other tools, so you would probably be best off moving it to webpack config.

trotzig commented 8 years ago

That whole module is causing problems... It's a thing that slides down from the top of the page telling you to install a native iphone app. Not something I'm a fan of... :)

I'll look into moving it. We have other cases where we want to use loader prefixes though. I think it might be worth looking at a webpack environment where these things can be sorted out somehow.