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

ESM Bundled Output still uses dynamic requires. Unable to make bundled, module safe output #1944

Open
aphex opened this issue Jan 18, 2022 · 11 comments

Comments

@aphex
Copy link

aphex commented Jan 18, 2022

Using a very simple npm package as an example, like jwt-simple given the following input

import JWT from 'jwt-simple'
console.log(JWT)

running this through esbuild with bundle and format ESM set we will get something like this

// node_modules/jwt-simple/lib/jwt.js
var require_jwt = __commonJS({
  "node_modules/jwt-simple/lib/jwt.js"(exports, module) {
    var crypto = __require("crypto");
    ```

However ideally what we would want here is `__require` should be transformed to a `async import` something like this
```js
// node_modules/jwt-simple/lib/jwt.js
var require_jwt = __commonJS({
  async  "node_modules/jwt-simple/lib/jwt.js"(exports, module) {
    var crypto = await import("crypto");

Currently it is not possible to use esbuild to bundle a project that imports npm packages that still use cjs. I would understand if the npm package itself had a dyanmic require in it. That should throw an error, but esbuild should not be generating dynamic requires in its bundles if want to target esm. Maybe this needs to be another option? Some way to output module safe bundles code? Though I would think if a user specifies a format of ESM, they would want to use import over require.

I believe this is connected to #1921

@kzc
Copy link
Contributor

kzc commented Jan 18, 2022

var crypto = await import("crypto");

A dynamic import() + await is less efficient than a static import for side-effect-free node built-in modules:

import crypto from "crypto";

This is effectively what the script in #1921 generates with esbuild --platform=node --format=esm output.

This functionality ought to be relatively easy to integrate into esbuild itself as it already did all of the upfront analysis.

@aphex
Copy link
Author

aphex commented Jan 18, 2022

oh i see, so would your suggestion be moving all dynamic requires to static imports? Any possible issues with modules crossing over with each other? say 2 different packages have the same dynamic require but different versions? Maybe you were suggesting static imports for node modules only?

Either way is good with me, though I am just gonna stick with cjs for now as I would rather not post process with a script. Hopefully esbuild can provide an option for this though.

@hyrious
Copy link

hyrious commented Jan 18, 2022

It is more incorrect to transform a require to dynamic import than bare import because then you have to mark all related functions async, which we called function color. The script in #1921 is a good workaround. And don't worry if you're generating import statement in cjs context, esbuild can still transform them to require call.

I have made a plugin using the same trick, you may have a look: https://github.com/hyrious/esbuild-plugin-commonjs. Through witch 1. it is not post processing your code, 2. no versions problem since the import statement is injected to the module's content.

@aphex
Copy link
Author

aphex commented Jan 18, 2022

I definitely see where your coming from but almost all of this code is being generated to begin with so I think its fair to change how it is assembled. I can see how messing with how someone's code works during transpile could get touchy though.

However I could also see a case where moving to a bare import would be not what a developers wants. For example a module that does some heavy amount of work it its module scope. For example compiling templates or something upfront. Now you have to pay for that compile time even if you never have a code path to that file. In Lambda I could see this coming up more as you would need to pay to run compilation on things that may never even happen. If you go to async imports you would pay that cost when you do the dynamic import.

Either way it would be nice if esbuild understood esm output OOTB. Whichever they land on, be it bare imports, or async, I trust they will work through the cases. Heck maybe we need to be able to configure which type of transform we would like to use, async or bare.

I will take a look at the plugin though, thanks for putting that together! This could be a good way to get by until esbuild can work this in. 🙏

@dougmoscrop
Copy link

dougmoscrop commented Jan 27, 2022

If you're trying to generate an esm bundle with cjs sources, createRequire() could be used in a banner

Consider an entrypoint, ulid.js like:

import { ulid } from 'ulid'

console.log(ulid())

This will throw Error: secure crypto unusable, insecure Math.random not allowed

Adding a banner to the build:

{
    entryPoints: ['./ulid.js'],
    bundle: true,
    platform: 'node',
    format: 'esm',
    outfile: './ulid.bundle.mjs',
    banner: {
        js: [
            `import { createRequire as topLevelCreateRequire } from 'module'`,
            `const require = topLevelCreateRequire(import.meta.url)`
        ].join('\n')
    }
}

This will still cause actual dynamic requires (for example, require(process.env.THING_TO_REQUIRE)) to fail, but I mean, in both cases it's a runtime error.

I wonder if esbuild could do this automatically if platform is node and the thing being passed to __require is a builtin.

@aphex
Copy link
Author

aphex commented Jan 27, 2022

Thanks @dougmoscrop this is a pretty great solution. It would be great if esbuild had a more official option to output a ESM compatible bundle and all it did was inject this into the top. So I am gonna leave this open in hopes this makes it was into a proper config.

For now though this is very helpful. Thanks!

@huvber
Copy link

huvber commented Jul 25, 2022

The solution of @dougmoscrop works only on platform:node since there would be possible to retrieve the module package. It won't work on browser environment. Do we have a solution for this scenario?

@acao
Copy link

acao commented Jul 26, 2022

same here, createRequire only works for me in node. and what's more, for other reasons, this solution does not work in cloudflare workers runtime at all (at least not in wrangler dev --local)!

without node_compat:
Screenshot 2022-07-26 at 10 30 07

@guushamann
Copy link

guushamann commented Mar 31, 2023

for me this little config resolved all my problems with require in packages, and the use of __dirname and __filename in packages. tested and a middle size project and deployed


import * as esbuild from 'esbuild'

await esbuild.build({
    entryPoints: ['src/main.ts'],
    bundle: true,
    outfile: 'dist/main.js',
    format: "esm",
    target: "esnext",
    platform: "node",
    banner:{
        js: `
        import { fileURLToPath } from 'url';
        import { createRequire as topLevelCreateRequire } from 'module';
        const require = topLevelCreateRequire(import.meta.url);
        const __filename = fileURLToPath(import.meta.url);
        const __dirname = path.dirname(__filename);
        `
    },
})


@strogonoff
Copy link

@guushamann Thanks for sharing. The banner that worked for me (I believe your version is missing path import):

        import { fileURLToPath } from 'node:url';
        import { dirname } from 'node:path';
        import { createRequire as topLevelCreateRequire } from 'node:module';
        const require = topLevelCreateRequire(import.meta.url);
        const __filename = fileURLToPath(import.meta.url);
        const __dirname = dirname(__filename);

I was hoping that this banner will finally let me bundle esbuild itself, but it only got me to runtime error “The esbuild JavaScript API cannot be bundled”, and I guess that’s that. :(

@Venipa
Copy link

Venipa commented Aug 27, 2024

import { fileURLToPath } from 'url';
import { createRequire as topLevelCreateRequire } from 'module';
const require = topLevelCreateRequire(import.meta.url);

import { fileURLToPath } from 'url';
import { createRequire as topLevelCreateRequire } from 'module';
import { dirname as ddirname } from 'path';
const require = topLevelCreateRequire(import.meta.url);
const __filename = fileURLToPath(import.meta.url);
const __dirname = ddirname(__filename);

alias to dirname to avoid var name conflict

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

9 participants