module2.require is not a function when bundling for node

[mhart]

Given:

test.js:

require('./test2')

test2.js:

console.log(typeof module.require('crypto'))

And running:

$ node test.js 
object

$ esbuild --version
0.7.15

$ node <(npx esbuild --bundle --platform=node test.js)
/dev/fd/11:11
  console.log(typeof module2.require("crypto"));
                             ^

TypeError: module2.require is not a function

When I look at the bundled output, it seems that no arguments are passed in to require_test2:

var __commonJS = (callback, module2) => () => {
  if (!module2) {
    module2 = {exports: {}};
    callback(module2.exports, module2);
  }
  return module2.exports;
};

// test2.js
var require_test2 = __commonJS((exports, module2) => {
  console.log(typeof module2.require("crypto"));
});

// test.js
require_test2();

This pattern is used in the real world, for example here:

https://github.com/apollographql/apollo-server/blob/570f548b88750a06fbf5f67a4abe78fb0f870ccd/packages/apollo-server-core/src/utils/createSHA.ts#L5-L7

I've also tried passing --define:module.require=require and --define:module2.require=require but neither of them seem to have any effect (should I open a separate bug for that?)

[evanw]

This pattern is used in the real world, for example here:

That's the first time I've seen something like this. It looks like they are deliberately trying to mess with the bundler's import logic. Perhaps module.require should just be treated as an alias for require? Then using --external:crypto (or --platform=node, which implies --external:crypto) would work here.

I've also tried passing --define:module.require=require and --define:module2.require=require but neither of them seem to have any effect (should I open a separate bug for that?)

This is because --define only works on unbound variables to avoid unintentionally overriding local variables that happen to be named the same thing. It's somewhat confusing here but module is technically not an unbound variable because of how node loads code using a function(exports, require, module, __filename, __dirname) wrapper.

Perhaps --define could treat these wrapper argument variables as globals for the purpose of matching. Although that may not be necessary if --define:module.require=require is essentially built in as proposed above.

[mhart]

Perhaps module.require should just be treated as an alias for require?

I certainly think that's true for --platform=node, yeah. Not sure how you'd want to handle in other envs

[mhart]

My only concern about just changing module.require is: wouldn't any other use of the module variable still have the same problem? Or is it specifically just module.require that's likely to be broken here?

[evanw]

wouldn't any other use of the module variable still have the same problem?

Yes. Because of module.exports, esbuild needs to intercept the module variable. However, esbuild deliberately doesn't handle the full complexity of file system emulation in the resulting bundle so properties such as module.paths will not work as expected.

Even module.require won't work as expected because according to the documentation it's supposed to be a way to require relative to that module. So I assume you can capture module.require and pass it to another module which can then call require in the original module's context? If that's true then when both modules are bundled, the original module's context no longer exists and it's not possible to emulate module.require correctly. In that case replacing module.require with require isn't really correct.

I think the easiest way to fix this particular package would actually be to forward module.require to require at run-time. It won't be a correct implementation but it will at least obey the intent of this package: to bypass the bundler's require function.

[guybedford]

module.require and require are semantically equivalent. This is why webpack created __non_webpack_require__ for the host require. This forwarding implemented is incorrect.

…

On Tue, Oct 13, 2020 at 22:25 Evan Wallace ***@***.***> wrote: Closed #455 <#455> via 9dbe4c8 <9dbe4c8> . — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#455 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSQKXI4UV4DQ4GCQSY3SKUY4FANCNFSM4SPN5IHA> .

[guybedford]

The confusion might be that the ‘module’ object identity is never typically shared in the CJS ecosystem between modules such that the common case always remains contextual.

…

On Wed, Oct 14, 2020 at 03:51 Guy Bedford ***@***.***> wrote: module.require and require are semantically equivalent. This is why webpack created __non_webpack_require__ for the host require. This forwarding implemented is incorrect. On Tue, Oct 13, 2020 at 22:25 Evan Wallace ***@***.***> wrote: > Closed #455 <#455> via 9dbe4c8 > <9dbe4c8> > . > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#455 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAESFSQKXI4UV4DQ4GCQSY3SKUY4FANCNFSM4SPN5IHA> > . >

[evanw]

One problem is that module.require is a dynamic property access while require is a statically-bound identifier. For example, code might do this:

var m = module;
m.require('./file');

I assume Webpack can handle this because it can emulate the file system in the bundle, but emulating the file system is a non-goal in esbuild.

[mhart]

Given that the examples we've seen so far have specifically been designed to stop bundlers from doing anything, is there a way to just leave such usages of module alone?

[guybedford]

@evanw the assumptions are mostly heuristical conventions. I've never seen arbitrary module assignment like that in the wild. Module construction is used for custom require virtualization (new Module() cases, before the createRequire API was provided). Otherwise there are just some old uses of module.require since that is an equal pattern to require. Sharing a module binding between modules I've never seen either as a pattern, or if it is it will be loader-like cases that are highly Node.js specific. module.require as an alternative to require is not an uncommon case to support though.

[mhart]

Btw, @evanw guess what I just spotted 😉

https://github.com/evanw/node-source-map-support/blob/d29e9c81527346b6ec385f7ba27a7a1c487b69c7/source-map-support.js#L17-L25

[guybedford]

It all makes sense now :)

Note that a scoped module binding is very different to the contextual one of course!

[mhart]

Hmmm? This is still just doing module.require('worker_threads')

https://github.com/evanw/node-source-map-support/blob/d29e9c81527346b6ec385f7ba27a7a1c487b69c7/source-map-support.js#L574-L575

[evanw]

Oh, interesting. Someone else has been maintaining source-map-support and I haven't been following along. It looks like this change was made to fix a problem with Webpack: evanw/node-source-map-support#270.

Unrelated, I also found a similar open issue for Parcel: parcel-bundler/parcel#3776. It looks like this library actually expects module.require to be undefined in a bundler, otherwise it attempts to call it. So perhaps the fix I landed isn't the general solution I had hoped it would be, since it would cause this package to crash in the browser. I'm going to revert my fix for now and reopen this issue.

Really the problem is that these packages aren't following the recommended way to do this, which is to use the browser field in package.json. There is consensus among the major bundler authors here that this is the recommended approach: webpack/webpack#8826 (comment).

Otherwise there are just some old uses of module.require since that is an equal pattern to require.

Do you have any examples of such code? I was unable to find any examples that don't just want the host's require function. Unfortunately GitHub code search is terrible in this case.

[guybedford]

There was an earlier version of moment that used this pattern that I've had to fix it for in the past. This was used to dynamically require locales, so it was doing:

module.require('./locale/' + name);

and the tool I was working on at the time was designed to handle these cases via globbing analysis.

So I guess in theory it was an attempt to use it as the opt-out actually. But it's not really a reliable opt-out unless we want to explictly decide to make it one and actively drive that.

If it's only explicit CallExpression cases for the unbound scope I think it's fine - it would still be undefined in other cases.

In short, if esbuild aims to build for Node.js equally then it probably should detect it. Otherwise it could be ok to leave out.

[evanw]

Note to self: I think a better version of my initial approach would be something like this:

 export var __commonJS = (callback, module) => () => {
   if (!module) {
-    module = {exports: {}}
+    module = __platform === 'node' ? {exports: {}, require} : {exports: {}}
     callback(module.exports, module)
   }
   return module.exports
 }

That way module.require would be the host's require function when targeting node and undefined when targeting the browser.

[guybedford]

@evanw I hope we can treat building for Node.js ES modules as first class.

[evanw]

@evanw I hope we can treat building for Node.js ES modules as first class.

What do you mean by that?

[guybedford]

As in not needing to rely on require ever existing in the environment at all even for Node.js builds in a modern setting.

[evanw]

Ah, I see. Do you mean that you'd expect esbuild to polyfill require using something like this when the output format is esm and the platform is node?

import { createRequire } from 'module'
var require = createRequire(import.meta.url)

Actually, why doesn't that already happen in node? Why is require not just in scope automatically? Should I avoid using require in esm?

[mhart]

@evanw what's your stance on just leaving module.X as is, for anything that's not module.exports?

[mhart]

FWIW, lodash does similar things to the examples we've seen so far trying to get at Node.js built-ins: https://github.com/lodash/lodash/blob/898b378f069ecb6c92b7713529985ba78ff34d31/.internal/nodeTypes.js

[guybedford]

@evanw I think avoiding require internalization is ideal. Even avoiding createRequire is ideal. It's not always possible, but in many cases there are techniques. For example the require('./locales/' + x) case can be based on globbing as discussed.

createRequire is a problem because no bundlers yet support it. So you have code that runs in Node.js, but won't run anywhere else.

The benefit of converting CommonJS modules into full ES modules that are platform independent is the same build techniques can then be used to eg virtualize CommonJS modules for Node.js in environments like Deno. The universal semantics and all. Eg converting createRequire cases into ESM where possible without relying on a platform require.

There are of course cases that don't convert, and then falling back to some environment require gets the final compatibility. But right up until that point to build for ESM entirely without relying on an environment require helps the ecosystem IMO.

Lines to be drawn though of course, but I always push for the universal side when possible.

[evanw]

I just did a survey of the major bundlers. Should have done that at the start, sorry. Here's what I tested:

// index.js
require('./nested')

// nested.js
function req(mod, path) { return mod.require(path) }
console.log(`require('fs'):`, !!require('fs'))
console.log(`module.require('fs'):`, !!module.require('fs'))
console.log(`mod.require('fs'):`, !!req(module, 'fs'))

Bundler	require('fs')	module.require('fs')	mod.require('fs')
Webpack	✅	✅	❌
Browserify	✅	❌	❌
Parcel	✅	❌	❌
Rollup + CommonJS plugin	✅	❌	❌

Webpack appears to be unusual in that it replaces module.require() with require() in the generated code. This behavior seems really strange to me on Webpack's part, but I suppose I should implement module.require as a literal alias for require for Webpack compatibility. As far as I can tell this is the commit where this behavior originated: webpack/webpack#7750.

@evanw what's your stance on just leaving module.X as is, for anything that's not module.exports?

Right now I am leaving module.X as is. There is currently no special behavior in esbuild for the syntax module.X. However, in a CommonJS file the module variable is a parameter of the generated CommonJS wrapper function for that file in the bundle. So leaving module.X as is means module refers to the generated module object in the bundle instead of the host's module object. As you can see from the chart above, this behavior where module is not the host's module object is consistent with all major bundlers (the last column of the table). So esbuild should do this as well to match the other bundlers.

[mhart]

Right now I am leaving module.X as is.

Well, you're not. It's being changed to module2 and this variable is passed in to a wrapper function.

I meant literally leaving it as module.X in the code so that it refers to the actual module variable that Node.js will inject when it requires the bundle.

[mhart]

Can confirm this no longer throws in 0.7.16 – thanks @evanw !

[ursi]

I am encountering code that uses ... && module.require && ... as a check to determine if it's being run in the browser or in node, and as a result the bundled code fails to run on node.