evanw / esbuild

An extremely fast bundler for the web
https://esbuild.github.io/
MIT License
38.14k stars 1.15k forks source link

ESM Format contains external `require` calls #1232

Closed DeMoorJasper closed 3 years ago

DeMoorJasper commented 3 years ago

When running esbuild with "format": "esm" it produces an invalid esm bundle by not transforming require calls into imports.

This only happens if there are external modules like with "platform": "node"

Minimal repro:

await esbuild.build({
  bundle: true,
  platform: "node",
  format: "esm",
});
import * as realpath from "fs.realpath@1.0.0"

console.log(realpath);
evanw commented 3 years ago

This transformation is not done because an import statement is not equivalent to a require call (they run at different times, and require calls may not run at all while import statements always run). Doing this transformation would change the meaning of the code. You can work around this by aliasing require yourself since node doesn't do this automatically:

 await esbuild.build({
   bundle: true,
   platform: "node",
   format: "esm",
+  banner: {
+    js: `
+      import {createRequire} from 'module'
+      const require = createRequire(import.meta.url)
+    `,
+  },
 });

Edit: The problem is that there is no equivalent to require in native ESM syntax. There is import but it returns a promise, and you can't just use await import because you may not be in an async context.

evanw commented 3 years ago

Closing as a duplicate of #946.

guybedford commented 3 years ago

This is an important use case in terms of having well-defined support for externalization membranes so would recommend reconsidering the decision here.

When building libraries that want to externalize dependencies of dependencies, they might not have control over how the import was made in the first place - whether it was an import or require.

We have the simple rule that can be applied here and that is that CJS modules are always just a default export so that that externalization can make complete sense.

Yes that's not comprehensive but more advanced corrections can be made over time or options can be provided to have fine-grained control further like RollupJS and Webpack offer as well. Eg via inspection functions on the namespace import in this case.

evanw commented 3 years ago

CJS modules are always just a default export so that that externalization can make complete sense

The problem is that the evaluation order is wrong, not that the external modules are CommonJS. For example:

exports.lazyImport = () => require('x')

It's not equivalent to compile that to this code:

import x from 'x'
export let lazyImport = () => x

It's also not equivalent to compile that to this code:

export let lazyImport = () => import('x').then(x => x.default)

There's no ESM equivalent of require(). You already know this; I'm just saying that I'm not sure "externalization can make complete sense" because they aren't equivalent. The most accurate way to represent the original code is to keep the CommonJS require, although that's node-specific:

import {createRequire} from 'module'
const require = createRequire(import.meta.url)
export let lazyImport = () => require('x')

would recommend reconsidering the decision here

There was no decision made here other than to close this as a duplicate of #946 (which is still open). I agree that this is something to figure out, but this is already tracked by #946 and I'd like to try to keep the conversation in one place instead of having multiple issues.

guybedford commented 3 years ago

@evanw externalization always causes reordering, eg for import './local.js'; import 'external', 'external' effectively being hoisted. I think it's an accepted part of the semantics of externalization at this point and similarly for CommonJS lazy imports no longer being lazy. I didn't see #946 was an identical issue, that seems fine to track further. Those ilisted workarounds all give me headaches too...

maartenbreddels commented 8 months ago

and require calls may not run at all while import statements always run

For this, can esbuild not translate this to import function calls