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

[FeatureRequest] Allow users to specify which modules are commonjs #447

Closed
shrinktofit opened this issue Oct 10, 2020 · 5 comments
Closed

Comments

@shrinktofit
Copy link

shrinktofit commented Oct 10, 2020

Currently, as code suggested, files without ES6 syntax are treated as CommonJS modules :

https://github.com/shrinktofit/esbuild/blob/1d9348337a01efe2eb79eb4f2ea78f1af313b38d/internal/bundler/linker.go#L1025-L1027

This may not be a good idea.

Empty files can be easily produced by TypeScript if source files contain type-only declarations. Another cases includes "import execution":

import "./to-exec.js"; // to-exec.js contains only execution logic for example console.log

esbuild will generate _commonjs wrapper for CommonJS modules, even unexpectly, modules that transitively exporting star from CommonJS modules are also considered as CommonJS modules. By an entry point doing so, that entry point will only export a default binding, even if in actual it exports many named bindings.

I would suggest the following rules:

  • By default, all files are treated as ES6 module files;
  • User may specify a regex, or glob, to tell which modules are commonjs.

Related: #442

@evanw
Copy link
Owner

evanw commented Oct 10, 2020

For context: this current behavior is inspired by TypeScript, which does the same thing.

I'm wary of introducing lots of configuration options because it can easily get to be overwhelming. In general I'm interested in exploring avenues for figuring stuff out automatically if possible before adding configuration options.

One could argue that there could be automatic solutions as well. For example, if a module doesn't contain import or export but also doesn't use the CommonJS module and exports variables, then I think it could just be safely treated as an ES6 module with any imports being automatically rewritten to undefined?

Another solution, which aligns with large parts of the JavaScript ecosystem such as browsers and node, is to have a system of rules to specify which modules are which type (called the "goal" of the parser in the specification). For example, in node ES6 modules must be marked in package.json and CommonJS modules are forbidden from importing ES6 modules. I didn't take this approach because I think this distinction is an unfortunate complexity that users of esbuild shouldn't have to deal with. It could also cause subtle bugs and incompatibilities when some configuration is wrong somewhere and the module is accidentally treated as the wrong type.

However, I don't think any work should be done on this before understanding the real-world implications. The bundler is already complex enough and I don't want to just accumulate complexity for complexity's sake. Did this come up because you have a problem with a specific library that's not working? Or is this just a theoretical concern?

@shrinktofit
Copy link
Author

shrinktofit commented Oct 10, 2020

For example, if a module doesn't contain import or export but also doesn't use the CommonJS module and exports variables, then I think it could just be safely treated as an ES6 module with any imports being automatically rewritten to undefined?

Agree with this. It seems like what babel does.

Did this come up because you have a problem with a specific library that's not working?

Our library has some type-only .ts. Some of the entry points transitively export * from them. This leads to those entry points only export a default binding.

@evanw
Copy link
Owner

evanw commented Oct 13, 2020

The behavior I described has now been implemented. From the release notes:

Previously TypeScript files could have an unnecessary CommonJS wrapper in certain situations. The specific situation is bundling a file that re-exports something from another file without any exports. This happens because esbuild automatically considers a module to be a CommonJS module if there is no ES6 import/export syntax.

This behavior is undesirable because the CommonJS wrapper is usually unnecessary. It's especially undesirable for cases where the re-export uses export * from because then the re-exporting module is also converted to a CommonJS wrapper (since re-exporting everything from a CommonJS module must be done at run-time). That can also impact the bundle's exports itself if the entry point does this and the format is esm.

It is generally equivalent to avoid the CommonJS wrapper and just rewrite the imports to an undefined literal instead:

import {name} from './empty-file'
console.log(name)

This can be rewritten to this instead (with a warning generated about name being missing):

console.log(void 0)

With this release, this is now how cases like these are handled. The only case where this can't be done is when the import uses the import * as syntax. In that case a CommonJS wrapper is still necessary because the namespace cannot be rewritten to undefined.

Does this behavior solve your issue? If so, I'd like to close this even though there is no way to specify which modules are CommonJS given that the underlying issue has been resolved.

@evanw
Copy link
Owner

evanw commented Oct 16, 2020

I'm going to close this since the underlying export * from should be fixed now.

@evanw evanw closed this as completed Oct 16, 2020
@EnderShadow8
Copy link

I'm working with a library which can either be consumed in a CJS or IIFE format. I'm importing it as an IIFE using ESM's static import syntax but esbuild treats it like a CJS module because of the prescence of CJS module and exports keywords. Can this issue be reopened?

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