Code which relies on checking require at runtime to determine the env (node/browser) is not working with esbuild

[ruanyl]

I'm not sure if this is esbuild's expected behavior or not, I've checked several other similar issues, but none answered my doubts.

The issue is when I bundle this package: uniqid, it has code like this to conditionally require node builtin modules

if(typeof __webpack_require__ !== 'function' && typeof require !== 'undefined'){
    var mac = '', os = require('os'); 
    if(os.networkInterfaces) var networkInterfaces = os.networkInterfaces();
    if(networkInterfaces){
        loop:
        for(let interface_key in networkInterfaces){
            const networkInterface = networkInterfaces[interface_key];
            const length = networkInterface.length;
            for(var i = 0; i < length; i++){
                if(networkInterface[i] !== undefined && networkInterface[i].mac && networkInterface[i].mac != '00:00:00:00:00:00'){
                    mac = networkInterface[i].mac; break loop;
                }
            }
        }
        address = mac ? parseInt(mac.replace(/\:|\D+/gi, '')).toString(36) : '' ;
    }
} 

With esbuild:

esbuild.buildSync({
  entryPoints: ["uniqid"],
  target: "esnext",
  format: "esm",
  outfile: "./dist/uniqid.esm.js",
  platform: "browser",
  bundle: true,
  external: ["os"],
});

Getting the runtime error:

 Error: Dynamic require of "os" is not supported

This is the build output, esbuild created a __require function for runtime:

var __require = /* @__PURE__ */ ((x) => typeof require !== "undefined" ? require : typeof Proxy !== "undefined" ? new Proxy(x, {
  get: (a, b) => (typeof require !== "undefined" ? require : a)[b]
}) : x)(function(x) {
  if (typeof require !== "undefined")
    return require.apply(this, arguments);
  throw new Error('Dynamic require of "' + x + '" is not supported');
});

// ......
    // <---- __require here is no longer the global "require" but the __require defined above
    if (typeof __webpack_require__ !== "function" && typeof __require !== "undefined") {
      mac = "", os = __require("os");
// ......

My doubt is what's the intention of creating a "__require" function for runtime? It seems it's a wrapper to output a nice error message when "require" is not exist, is it?

[thdxr]

This is the same issue as #1944 with an ugly hack in #1921

[ruanyl]

@thdxr Thank you for your reply, but #1944 doesn't seem like the same issue, issue #1944 is more about cjs to esm converting in the output which I believe esbuild does't support yet.

My point here is: creating a wrapper of require potentially breaks all modules that check global require at runtime to do conditional imports in the way like uniqid.

[evanw]

This is by design because esbuild takes modules in CommonJS format as input, and the CommonJS format has the following invariants:

• require is always a function
• exports is always an object
• module is always an object
• module.exports always starts off being equal to exports

It doesn't matter if the code is running in the browser or node or somewhere else. Code in the CommonJS format should be able to rely on the invariants that come with the CommonJS format.

I recommend testing whether code is running in the browser or not using something like window, which is not an aspect of the CommonJS format.

[ruanyl]

Thanks, @evanw. Unfortunately, uniqid is a dependency of a dependency that we use that we cannot control. And I believe there are other packages that are doing such runtime checking as well.

If I understood correctly from what you say, code like typeof require !== 'undefined' will become "invalid' code(with esbuild's current design) as for commonjs format, require is always a function even when the code is run in the browser. So it doesn't make sense to compare an always existing value with undefined.

Does it make sense to only replace the require with __require at where require function is invoked, but NOT replace where require is used as an expression like typeof require !== 'undefined'? So that it won't affect the runtime behavior.

[hyrious]

You can still have a workaround to the uniqid package by writing plugins. It may look like this:

name: 'fix-uniqid',
setup({ onLoad }) {
  const fs = require('fs')
  onLoad({ filter: /\buniqid[\\\/]index\.js$/ }, async args => {
    let code = await fs.promises.readFile(args.path, 'utf8')
    code = code.replace(
      /if\s*\(typeof\s+__webpack_require__\b[^]+\/\/\s*Exports/m,
      '/* erased */'
    )
    return { contents: code }
  })
}

By the way, the correct way to export a cross-node/browser package now is using the "browser" field in package.json, you may have a look.

[geoffharcourt]

I've gotten around this by using os-browserify for some similar dependencies that I don't control:

In package.json:

  "browser": {
    "os": "os-browserify",
    "stream": "stream-browserify"
  },

[ruanyl]

@hyrious Thanks for the tips, but actually I'm creating an ESM dev server for unbundled development with esbuild. So I don't want to go with anything too specific. For packages with browser field, esbuild works very well. But unfortunately , not all packages are in the modern way.

I know esbuild is not designed to be compatible with all the conventions/rules in the JS ecosystem(which I really appreciate a lot). But just does not feel all right in this case which changed the code behavior at runtime.

[evanw]

Does it make sense to only replace the require with __require at where require function is invoked, but NOT replace where require is used as an expression like typeof require !== 'undefined'? So that it won't affect the runtime behavior.

No, I don't think so. That would introduce inconsistency into the code and means the require variable would take on semantics that are outside of JavaScript (both being a function and undefined at the same time), which is very confusing in my opinion and could potentially lead to bugs. For example, one such case is #1202. Someone wrote code like this:

(function (dirtyChai) {
    if (typeof require === 'function' && typeof exports === 'object' && typeof module === 'object') {
      module.exports = dirtyChai;
    } else if (typeof define === 'function' && define.amd) {
      define(function () {
          return dirtyChai;
      });
    } else {
      chai.use(dirtyChai);
    }
}(function dirtyChai(chai, util) {
    ...
}));

Due to esbuild previously doing exactly what you described (leaving typeof require alone), this code had a bug where after being bundled with esbuild it failed to detect that CommonJS was supported and crashed at run-time. Code like this is entirely reasonable in my opinion because it's just checking at run-time that it should use CommonJS-style exports and esbuild supports CommonJS-style exports, so this test should always pass. This is why I changed esbuild to include the shim function, which ensures that typeof require === 'function' is true and that this code doesn't crash.

[ruanyl]

Ok, thanks for the context. So the old way breaks the conditional exports and the current way doesn't work well with the conditional require.

I tested how webpack deals with typeof require !== 'undefined', and esbuild is aligned with webpack, I'll close the issue as it's expected behavior of esbuiid.