Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for required modules #2

Closed
LarsDenBakker opened this issue Jun 23, 2020 · 10 comments
Closed

Support for required modules #2

LarsDenBakker opened this issue Jun 23, 2020 · 10 comments

Comments

@LarsDenBakker
Copy link
Contributor

Would it be possible to support looking up a commonjs module's required dependencies? I'm thinking of how I could use cjs-module-lexer for fast on the fly cjs-esm transformation.

Require calls are dynamic, so you can't statically analyze everything. But supporting some common use cases would be interesting.

@LarsDenBakker
Copy link
Contributor Author

Sorry to open a feature request on a fresh repository. This is great work as usual :) I'd contribute, but I don't have any experience with C unfortunately.

@guybedford
Copy link
Collaborator

Hey @LarsDenBakker, glad to hear you find the project useful.

Are you mainly referring to getting access to eg require(dynamicExpression()) sort of case, or the process of handling recursive dependency analysis here?

We could definitely add integration for arbitrary require expression detection. Possibly even with automatic conversion into wildcard expressions.

@LarsDenBakker
Copy link
Contributor Author

I was thinking of transforming CJS to ESM on the fly with a similar approach as the rollup commonjs plugin by analyzing the require and export statements, and generating imports/export for them.

We could use the rollup plugin for this, but doing AST analysis in javascript is slow. If we can use cjs-module-lexer to generate at least a basic analysis we could do the code transformation entirely with string concatenation with only a few ms overhead per file.

I'm fully aware of the commonjs-esm interop edge cases, and that require calls can by highly dynamic. But if we can just catch static top level require statements I think we can cover a lot of existing code already.

@guybedford
Copy link
Collaborator

I would actually strongly advise against this route - a large amount of handling of CommonJS conversion is actually related to scoping and strict mode conversion issues none of which can be handled in this way. So you'll get 80% there then hit a serious block with such an approach.

If your concern is performance, then eg move over to a native-based JS transformation tooling that can handle full scope analysis. The main issue on that side is finding reliable projects to build on, but there's some interesting stuff happening, and work there would have immediate wider benefit to the ecosystem without hitting a dead end as opposed to work on this.

@LarsDenBakker
Copy link
Contributor Author

Advice noted. But doesn't the rollup commonjs plugin have the same issues? I think it just forced all commonjs modules into strict mode.

Moving to native based tooling would be ideal, but I don't know of any per-file commonjs -> esm transform tool availably today. There is esbuild, but it only does it when bundling as far as I know.

@guybedford
Copy link
Collaborator

Yes, the rollup commonjs plugin has very bad levels of compatibility, but there is at least a path for it to improve - for example posting a bug for strict mode cases can be fixed with a relatively simple PR.

esbuild has a CommonJS layer specifically yes. Perhaps there's a way it could be extended to work to per-file by just bundling an individual CommonJS file? I'm sure @evanw would be interested in your use cases.

@evanw
Copy link

evanw commented Jun 29, 2020

There's already an open issue for esbuild about adding single-file module format conversions, which would allow conversion from CommonJS to ES6 modules: evanw/esbuild#109. It may not end up doing what you want though. The exports for CommonJS modules aren't statically analyzable in the general case so I'm not planning on doing the AST analysis that Rollup does. I'm planning to implement this conversion the way node does instead, which is that all CommonJS modules become an ES6 module with a single default export. That may not make it suitable for your use case.

@LarsDenBakker
Copy link
Contributor Author

It wouldn't work for some existing codebases, but I think it's still a great sensible way forward. I'll close this and watch the other issues then. Thanks!

@guybedford
Copy link
Collaborator

@evanw glad to hear you're matching proper Node.js interop semantics there, this has been an issue in other tools now trying to align.

You may also like to keep an eye on nodejs/node#33416 which is probably the closest Node.js has come to any consensus on named exports for CJS support (many proposals have been debated endlessly and this one has the least blocks).

So if named exports will land the discussion will be there (and this project is integrated in that PR which provides the basic heuristical wasm static analysis approach).

@evanw
Copy link

evanw commented Jun 29, 2020

Thanks for the pointer. I wasn't aware of that discussion going on. Good to know about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants