import-js icon indicating copy to clipboard operation
import-js copied to clipboard

Allow packages to define things that can be destructured

Open lencioni opened this issue 9 years ago • 19 comments

It would be nice if packages could add some data that would prevent consumers of the package from having to add a set of destructured properties to their import-js config. For instance, the set of things that could be destructured from underscore are pretty stable and don't really need to be defined for each individual consumer. Maybe this could be added to the package.json file or to a different file that is distributed with the package.

lencioni avatar Jan 08 '16 04:01 lencioni

That would be great! Who do we talk to? :)

trotzig avatar Jan 08 '16 11:01 trotzig

I think the first step would be for this plugin to catch on. Then, when a bunch of people are using it, we can add this feature and hope that packages will organically start supporting it.

Another option would be to allow people to write and share plugins that contain sets of destructurings for popular packages. The upside would be that would be more decentralized and more likely to actually support more packages. The downside is that would require more burden on consumers to find, install, and maintain the plugins.

Yet another option would be to have a directory of destructurings built into import-js, so it's just ready to go.

For any of these to work best, it might not be a bad idea to write a script that looks at a package and tries to identify the things that you might want to destructure. And, actually, if we get that to work really well and relatively fast, we could just have a daemon running that does this on the fly for any package.

lencioni avatar Jan 08 '16 17:01 lencioni

Regarding the @exports comments you suggested, I think that since the ESM syntax supports what we would need out of the box that we should at least start by only supporting ESM and not CommonJS if we want to go down this route.

lencioni avatar Jan 21 '16 04:01 lencioni

I've been thinking about this some more and I think that each project's package.json is a good spot for something like this. Here's a proposal.

What if we looked in package.json files of direct dependencies for an import-js key that would be an object with package specific configuration in it? That would allow the package to define things like named exports and additional entry points (e.g. for importing things like mypackage/some/other/thing, and other things that we might end up wanting in the future. Here's an example of what a package.json could look like:

{
  "name": "my-package",
  ...
  "import-js": {
    "namedExports": [
      "foo"
    ],
    "entryPoints": {
      "something/else": {
        "namedExports": [
          "bar"
        ]
      }
    }
  }
}

lencioni avatar Apr 30 '16 04:04 lencioni

Should we combine the two into a single object?

{
  "name": "my-package",
  ...
  "import-js": {
    "entryPoints": {
      "main": {
        "namedExports": "foo"
      } 
      "something/else": {
        "namedExports": [
          "bar"
        ]
      }
    }
  }
}

Perhaps we would need to prefix main with something to avoid collisions? __main, __default or something. Or we could just avoid that special keyword and have people duplicated the main string from the root config.

trotzig avatar May 02 '16 07:05 trotzig

I think maybe we should allow packages to specify an array of entry points, and then auto-resolve the named exports (#199) from the main file and the other entry points. It might be nice for these entry points to be glob patterns, so you can specify that whole directories should be considered entry points.

{
  "name": "my-package",
  ...
  "import-js": {
    "entryPoints": [
      "something/else",
      "exported/*"
    ]
  }
}

lencioni avatar Jul 14 '16 05:07 lencioni

This type of configuration seems like it could be useful for other tools than import-js, so we could even consider making it part of the root object. I don't know the process for getting things added to the package.json schema, and I think we'd probably have to get our tool more widely used before that can happen.

Also, did you mean for something/else to be without the file ending? I was expecting a .js there.

trotzig avatar Jul 14 '16 08:07 trotzig

This comment is not discussing recommendation as much as how another product is accomplishing some of this.

Meteor uses a package.js file that currently has two means of specifying the interface.

The older method uses api.export calls to explicitly define each package export. The example below states that 'SimpleSchema' and 'MongoObject' are exported to both the client and server builds and that 'humanize' is exported only in the test environment (similar to devDependencies).

  api.export(['SimpleSchema', 'MongoObject'], ['client', 'server']);
  api.export('humanize', {testOnly:true});

I sort of wished they had stayed with that because there is no need to parse any module (at least until we get into a world of typed interfaces). But I also understand their desire not to duplicate what ES6 now provides.

The new approach that came in with the implementation of ES6 modules is to specify a mainModule for each build. All exports in the mainmodule are then made available to be imported into that build.

  api.mainModule("server.js", "server");
  api.mainModule("client.js", "client");

In most cases I believe the packages will simply use a single entry point for all builds.

  api.mainModule("index.js");

They will then create an index.js that collects things from around the package and reexports them.

export { 'elementA' } from 'dirA/moduleA';
export { 'elementB' } from 'dirB/moduleB';

So this is the case where I realized I'll need to be able to find all of the namedExports in a module because they are no longer explicitly specified in the package.js.

rhettlivingston avatar Jul 14 '16 15:07 rhettlivingston

If import-js implemented entryPoints as specified above, I believe my task would devolve to processing the package.js files and producing packageDependencies, namedExport entries (in response to api.export calls) and entryPoint entries (in response to api.mainModule calls).

I'm working top down at the moment.

  1. identify packages
  2. find the package.js (local packages) or the build products that resulted from processing it (released packages)
  3. create packageDependencies for released packages.
  4. create namedExport entries for all occurrences of api.export
  5. merge namedExport entries extracted from mainModules.

I'm on step 1 and wasn't planning on writing something to extract exports from a module until my last step. If one of you beats me to it, awesome!

rhettlivingston avatar Jul 14 '16 15:07 rhettlivingston

I won't be able to get to it soon, so you'll probably beat me to it.

Are you planning on using regex or AST parsing? If you go down the route of AST parsing, we could maybe use that for some other things as well, such as finding the imports in the current file.

Whatever it is, it should be written in a way that will work for identifying named exports in local modules as well as package modules, of course.

lencioni avatar Jul 14 '16 18:07 lencioni

I'll likely start with regex. If time permits, I might also take a shot at AST. I am curious as to how bad the performance hit really is.

Yes, I will write a generalized routine that takes a module path and returns information about its exports including whether there is a default.

I believe that, ultimately, Meteor is going to demand that we do this for all local modules. The ES6 code that I've seen in the Meteor community almost never has default exports. So your generic means of matching the filename and then importing the default is rarely resulting in functional code.

On Thu, Jul 14, 2016 at 2:13 PM, Joe Lencioni [email protected] wrote:

I won't be able to get to it soon, so you'll probably beat me to it.

Are you planning on using regex or AST parsing? If you go down the route of AST parsing, we could maybe use that for some other things as well, such as finding the imports in the current file.

Whatever it is, it should be written in a way that will work for identifying named exports in local modules as well as package modules, of course.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Galooshi/import-js/issues/107#issuecomment-232746898, or mute the thread https://github.com/notifications/unsubscribe/AH5TYm705aEzN1z3WaSFedLfGlMVXRUuks5qVnxAgaJpZM4HA6gT .

rhettlivingston avatar Jul 14 '16 18:07 rhettlivingston

I've been thinking about this some more and I've thought of a couple tricky things that need to be kept in mind.

If we are trying to determine what named exports a module has, the ES6 export syntax is obviously a good place to start. However, we might also want to support the module.exports syntax for folks writing node code or just not using the newer syntax. This means that for code that module.exports an object, we would want to try to inspect the object's properties at the point of being exported. This might be too difficult to get right with a regex approach and even with AST it likely won't be perfect. If we decide to not support this, we might still need to provide a way for packages to specify this in configuration but we can cross that bridge when we get to it.

Also, some packages, such as react, look like this:

module.exports = require('./lib/React');

Alternatively, you could imagine a module simply importing or requiring something, and then exporting it again. Ideally we would be able to trace it back to the module being imported/required and determine the named exports on that object.

There's probably some code in https://github.com/benmosher/eslint-plugin-import that could help with this problem. Specifically code for the named rule: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/named.md

lencioni avatar Jul 14 '16 19:07 lencioni

Thank you. Not all of that has occurred to me. Interestingly, a lot of your concerns aren't CommonJS specific. They also occur in ES6 syntax. For example, I think your code above is the same as

export * from './lib/React';

and I've already mentioned that Meteor packages often use an index.js with similar syntax to create an interface that is cherry picked from a package's modules.

I think that I'm already going to have the task of digging out requires in Meteor, interestingly, in the case of finding the exports of 3rd party ES6 packages. In that case, I may have to mine the package build product which has already been transpiled.

Your pointing at eslint-plugin-import is a bit spooky. I just did a PR for that package to fix a Meteor-related error. I had already been thinking that there may be some code in that package with some applicability.

rhettlivingston avatar Jul 14 '16 19:07 rhettlivingston

👻

trotzig avatar Jul 14 '16 19:07 trotzig

In looking at the above "entryPoints" grammar after having delved deeply into Meteor's package interface definitions over the last few days (I have a 90%+ implementation of automatically defining Meteor package namedExports without examining code done), I note a few things.

  • There are two commonly defined entry points in the existing package.json, "main" and "browser" (added by browserify).
  • Unless we ignore CommonJS, everything I read is indicating to me that we won't be able to auto-resolve namedExports reliably. Maybe this is OK, maybe not.
  • If we don't auto-resolve, how would we say that an entry point has no "default" export? ES6 modules can utilize named exports only. Perhaps we'd just always require "default" to be specified as a name if it is provided? This would match the truth underlying the system.
  • Frankly, packages have a way of specifying all of their named exports already if they use ES6 (specifically designed for ease of static analysis) and follow the conventions intended by npm (one "main" entry point). I think it is a bad practice for a package to have more than one entry point for any particular build target. If they have code spread around, they should be gathering it into the main module using the "export {xxx} from 'something/somewhereElse';" syntax. So, what we're doing here is enabling CommonJS users to hang on and enabling packages that want to buck the system.
  • Meteor has a means of specifying that a module or export is only for a particular build platform (such as client vs. server). This is similar to npm "main" vs. "browser", but more extensive. So perhaps we need to be able to specify other fields for each of these interfaces?
  • Meteor also has a means of specifying that an export is only intended to be utilized in a test environment.

Also, shouldn't an interface definition standard include some means of specifying the types of each export and some snippet as to what it does in a manner that would allow an editor extension to easily display the info to the user or a lint tool to more easily verify types? I suppose editors can do some of this already if they can track down JDoc info.

And if any of the above extra info were added, perhaps it should be called "interfaces" instead of "entryPoint"s?

rhettlivingston avatar Jul 18 '16 01:07 rhettlivingston

Relevant to this discussion... I stumbled on an informative discussion in the Node community where they have basically decided that ES modules will only be able to import CommonJS modules as "default". They had initially planned to allow the named import syntax to pluck properties out of the modules.exports structure, but realized that there are issues with what "this" means inside those elements. Basically, it can't work. So, no named imports will be allowed by ES modules from CommonJS modules. This simplifies our work.

rhettlivingston avatar Aug 16 '16 21:08 rhettlivingston

I also had a thought on this issue today. TypeScript has a means of declaring a module's interface (and a lot more that we don't care about). The DefinitelyTyped repository of TypeScript definitions for commonly used modules has .d.ts files for a lot of things (including all but 4 of the current import-js dependencies, for example).

I took a quick look at the lodash.d.ts and see nice declarative export statements for all of its pieces.

...
declare module "lodash/uniqueId" {
   const uniqueId: typeof _.uniqueId;
   export = uniqueId;
}


declare module "lodash/upperCase" {
   const upperCase: typeof _.upperCase;
   export = upperCase;
}


declare module "lodash/upperFirst" {
   const upperFirst: typeof _.upperFirst;
   export = upperFirst;
}
...

I also looked at the underscore definition and see

declare module "underscore" {
    export = _;
}

I could imagine a future option that would cause our code to look for TypeScript definitions of the packages in our dependencies, grab them when available, and utilize them for their interface definitions.

rhettlivingston avatar Aug 16 '16 21:08 rhettlivingston

That's a very interesting discussion. Thanks for sharing.

The typescript angle is very interesting as well.

trotzig avatar Aug 17 '16 18:08 trotzig

You're welcome!

rhettlivingston avatar Aug 17 '16 19:08 rhettlivingston