Output a warning when encountering `__dirname`

[Prinzhorn]

I'm aware of #859, but this is not a request to add some sort of __dirname transformation, but to output a warning for the current behavior.

I'm currently trying to bundle a large Electron app for faster startup time (less require() calls at runtime). I've run into multiple issues with modules using __dirname. E.g. I already marked vm2 as external because it loads files at runtime (#495). In the vm2 case I was lucky enough to get an error at runtime, but really this needs to be a warning when bundling. With __dirname the bundling effectively changes the semantics of the code. I'm using Piscina to run worker jobs. Naturally I called my worker worker.js. But after bundling a worker task would never finish and also not error. Turns out Piscina also ships with a worker.js that loads my worker. After bundling with esbuild Piscina was loading my worker.js instead of it's own. This is a side effect that should definitely cause a warning. Right now I'm considering adding every single dependency as external and only bundle my own code, because I cannot know for sure if something inside my dependency tree will use __dirname in unexpected ways and causes funny runtime behavior. With a warning I could only make the packages external that use __dirname. Assuming I could even easily figure out which package is the problem from the warning (it could be in a file that is a third level dependency of a dependency).

I'm now doing this:

external: Object.keys(JSON.parse(readFileSync('./package.json')).dependencies),

but I'm losing a lot of benefit of the bundling.

[Prinzhorn]

Well, I guess this is related to #1155
I'm not sure who complained about those warning, but undefined runtime behavior is not acceptable 🤷 (this is not shrugging in your direction)
Maybe there needs to be more control over the logging/warnings (lint-like rule sets, e.g. no-dirname)?

[evanw]

Right now I'm considering adding every single dependency as external and only bundle my own code, because I cannot know for sure if something inside my dependency tree will use __dirname in unexpected ways and causes funny runtime behavior.

This is by far the safest option because there are many other aspects of node's API other than __dirname that esbuild also doesn't bundle seamlessly (e.g. __filename, require.cache, etc.). I recommend doing this if you want a "hands off" approach.

If you're looking for more of a "hands on" approach, then there are various ways of doing this. One way could be to use something like --define:__dirname=ERROR_DIRNAME_WAS_USED and then have your build script reject the build immediately if ERROR_DIRNAME_WAS_USED is present in the output directory.

Another way to do this is to try to write a plugin. One approach is shown below. It's a little hacky but you can detect whether or not an access for any given global is present or not by using the inject feature to replace it with something and then using the metafile feature to check and see if the replacement happened:

const noDirnamePlugin = {
  name: 'no-__dirname',
  setup(build) {
    const path = require('path')
    const injected = path.join(require('os').tmpdir(), 'inject-__dirname.js')
    require('fs').writeFileSync(injected, 'export let __dirname')

    const options = build.initialOptions
    options.inject ??= []
    options.inject.push(injected)
    options.metafile = true

    build.onEnd(result => {
      const metafile = result.metafile
      if (metafile)
        for (const output in metafile.outputs)
          for (const input in metafile.outputs[output].inputs)
            if (path.basename(input) === path.basename(injected))
              throw new Error(`Using "__dirname" is not allowed`)
    })
  },
}

This will fail the build if something uses __dirname. It doesn't tell you which file contains it, although you should be able to figure that out easily from inspecting the output file.

Maybe there needs to be more control over the logging/warnings (lint-like rule sets, e.g. no-dirname)?

I don't want esbuild to become a full-blown linter, since that's a big step up in complexity. I agree with you that these kinds of things should be warnings (that's what warnings are for) and that you should just turn warnings off if you don't want to see them. But I've tried similar things in the past and lots of people complain about them, which is not something I want to deal with. So my solution is to move them to --log-level=debug instead which is not enabled by default. I'd be willing to add a debug-level log for things like __dirname and __filename. If that results in too many logs, you can always use the API to filter the log for specific messages.

[Prinzhorn]

This is by far the safest option because there are many other aspects of node's API other than __dirname that esbuild also doesn't bundle seamlessly (e.g. __filename, require.cache, etc.). I recommend doing this if you want a "hands off" approach.

Is this something esbuild should be doing by default? E.g. a new nodeModules option that defaults to platform != 'node'? It would prevent bundling anything inside node_modules. Because esbuild will likely become more popular and with serverless and Electron, bundling of Node.js will also become more popular. That means it will become more likely that you've traded the "I'm annoyed by this warning" issues with "My build is broken at runtime and behaves unexpected". I personally don't care that much, because now I've learned that I should never bundle my dependencies and I'm good. But others will keep running into the same issue. At least for my projects it is simply not feasible to review my entire dependency tree (for every new version of every dependency) to see if it can safely be bundled. I think for Node.js bundling it should just be the default to not bundle them and maybe even have an include option (mutually exclusive to exclude) so you can opt into bundling of some dependencies (e.g. some fifth level dependency you've tracked down to be safe to bundle and also be a bottleneck without bundling). This would make Node.js bundling safe and work out-of-the box (e.g. native modules can't be bundled anyway). Ultimately Node.js bundling is quite different from browser bundling.

Thanks for your "hands on" approaches. I would additionally need to check if these files are in node_modules, because my own code uses __dirname (e.g. I still need to be able to point to a Worker file or point fastify-static to the folder to serve) but in a safe way (the worker is defined as an additional entry point and will still be in the same relative location after bundling and the static files are in the same relative location from both src and dist). But like I said I will stick with my current solution, I'm not into gambling.

I agree with you that these kinds of things should be warnings (that's what warnings are for) and that you should just turn warnings off if you don't want to see them. But I've tried similar things in the past and lots of people complain about them

I'm on the other end of the spectrum: esbuild should straight up refuse to compile these things and tell you to exclude them (and maybe have an option dangerouslyCompileAllTheThings: true for people that hate their jobs 😄 ). I mean we're talking about software and not painting a room, where you can get away with some rough spots and where "correctness" is subjective and not well defined. It's weird having to argue that code should do the same thing before and after bundling. If the bundler can't guarantee that it will, then please crash as loud as possible 🙏

I've seen multiple issues in this repo where you've commented things like "well actually this is totally valid code, see here is the spec" or "no we can't remove this unused import, because importing might have side effects, even if you don't use the import" so you seem to care about all the little details and doing things the correct way. That's why I'm a little confused why in this case it's the opposite.

[evanw]

I'm reopening this as the fix for this broke an Amazon product that didn't pin the version of esbuild they were using. There's more context in #3819.