When used with require() and node, does not honor "main" field in package.json if "module" is present

[mhart]

EDIT: Originally ran into this with node-fetch, but it seems to have surfaced a larger issue which is that esbuild doesn't prioritize the main field in package.json if you're using require() with node. This means that esbuild diverges from how Node.js/CommonJS would require the same package.

It appears from the comments that this is by design:

We support ES6 so we always prefer the "module" field over the "main" field.

esbuild/internal/resolver/resolver.go

Lines 272 to 280 in 99f587a

	// * The "main" field. This is what node itself uses when you require() the
	// package. It's usually (always?) in CommonJS format.
	//
	// * The "module" field. This is supposed to be in ES6 format. The idea is
	// that "main" and "module" both have the same code but just in different
	// formats. Then bundlers that support ES6 can prefer the "module" field
	// over the "main" field for more efficient bundling. We support ES6 so
	// we always prefer the "module" field over the "main" field.
	//

But this means that certain packages just don't work with require(), such as node-fetch:

To reproduce:

$ node --version
v12.18.3

$ npm install node-fetch

$ node -p "typeof require('node-fetch')"
function

$ node -p "$(echo "typeof require('node-fetch')" | npx esbuild --bundle --platform=node)"
object

Would be great if esbuild supported the same behavior as Node.js for require'ing modules – if not out-of-the-box, then possibly via a flag?

[mhart]

This could possibly be related:

node-fetch/node-fetch#450 (comment)

But I couldn't resolve it with just the extensions suggestion alone:

$ node -p "$(echo "typeof require('node-fetch')" | npx esbuild --bundle --platform=node --resolve-extensions=.js)"
object

[mhart]

Hmmm. It may be that esbuild is failing to resolve the main field in package.json. If I explicitly require node-fetch/lib/index then it works:

$ grep main node_modules/node-fetch/package.json                                                   
  "main": "lib/index",

$ node -p "$(echo "typeof require('node-fetch/lib/index')" | npx esbuild --bundle --platform=node --resolve-extensions=.js)"
function

In the latest node-fetch beta, they've fixed any ambiguity around file endings, so resolve-extensions will no longer be necessary:

https://github.com/node-fetch/node-fetch/blob/64c5c296a0250b852010746c76144cb9e14698d9/package.json#L3-L5

But esbuild has the same problem with failing to resolve main:

$ npm install node-fetch@3.0.0-beta.8

$ node -p "$(echo "typeof require('node-fetch')" | npx esbuild --bundle --platform=node)" 
object

$ node -p "$(echo "typeof require('node-fetch/dist/index.cjs')" | npx esbuild --bundle --platform=node)"
function

[evanw]

I just checked Parcel and Webpack and they also do the same thing. The problem is that modern bundlers prefer module over main because that leads to better tree shaking. It doesn't have to do with file extensions. The code require('node-fetch') pulls in the file in the module field, and that file contains export default async function fetch() {}. This means the require() call returns an object with the fetch function as the default property, since that's how modules work.

Edit: I see that is the same issue that node-fetch/node-fetch#450 (comment) is talking about. The solution is mentioned in the thread below that comment: use import fetch from 'node-fetch' instead of let fetch = require('node-fetch'). That results in a function in esbuild, Parcel, and Webpack without needing to configure any bundler-specific settings.

[mhart]

The linked comment suggested that for Node.js, it should use “main”, because that’s what Node.js uses. If we’re explicitly bundling for node, shouldn’t esbuild do the same thing?

…

Sent from my iPhone

On Sep 3, 2020, at 2:43 AM, Evan Wallace ***@***.***> wrote:  I just checked Parcel and Webpack and they also do the same thing. The problem is that modern bundlers prefer module over main because that leads to better tree shaking. It doesn't have to do with file extensions. The code require('node-fetch') pulls in the file in the module field, and that file contains export default async function fetch() {}. This means the require() call returns an object with the fetch function as the default property, since that's how modules work. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[mhart]

import fetch from 'node-fetch' doesn't work in Node.js (prior to ESM module support). So that's fine if someone's only bundling. But I'm not (I have shared code – sometimes it's run from Node.js directly, sometimes from bundled code).

Also, wouldn't that mean that any module in node_modules that's using fetch also won't bundle correctly? Users don't always have control over the code doing the require.

Is there a reason that you wouldn't use the main field of package.json if the code is specifically using require and the user has specified node? It feels like this would lead to the least amount of gotchas in a node environment.

[mhart]

Updated title to reflect what I suspect is the underlying issue here (which you hinted at: esbuild always prefers module over main even if the module is required)

[mhart]

FWIW, I know you used Webpack and Parcel as examples here, but they also just happen to have the same bug (with Webpack you can configure it, not sure about Parcel). Browserify and Rollup get it right out of the box.

Related:
webpack/webpack#5756
jashkenas/underscore#2865 (comment)
jashkenas/underscore#2852 (comment)

[evanw]

Doing what you suggested (picking main over module if you use require) would introduce a potentially more severe issue where you could end up with duplicate copies of the same module in memory if some code uses require and some code uses import. That could cause code to behave incorrectly. So I don't think this suggestion is actually a valid way to fix the problem.

The current JavaScript module situation is a mess honestly. It wasn't designed holistically and there's no behavior that works the best in all cases. The design of the module field wasn't fully thought through because nothing stops you from also importing a file inside a package. So bundlers that respect module could still end up with duplicate copies of a module in memory if code also imports a CommonJS file from the same package.

Some bundlers do have a setting to prefer main over module but AFAIK it's for all modules, not per-module, and doing that would effectively disable tree shaking for all modules (as well as potentially introduce incompatibilities with packages that need bundlers to use module over main). So it's not something you'd want by default and potentially not even something you'd want to turn on at all.

I'm not sure what the right solution is here. It doesn't seem like other bundlers have come up with a good fix either. Maybe fine-grained per-package "main field" ordering? Maybe the right solution is just for the node-fetch package to not set the module field?

[mhart]

Why don't you think Browserify and Rollup have a good fix? They seem to be doing the right thing AFAICT

[evanw]

I just checked Rollup and it also seems to result in an object instead of a function just like Parcel and Webpack. Note that to get Rollup to bundle anything, you need to configure it to use @rollup/plugin-commonjs and @rollup/plugin-node-resolve.

Browserify does result in a function but it looks like it maybe it just doesn't support ES modules at all? I'm not sure because I'm not familiar with that tool myself, but that would explain why it's a function instead of an object in Browserify.

Here are the commands I'm using: https://gist.github.com/evanw/a8d65017c7876e85861245af7821d038. Run npm i && make to run the tests.

I'm not sure what your exact situation is but if you're ok with the resulting bundle still depending on the node-fetch module, you could "fix" esbuild easily by passing --external:node-fetch. This would preserve the require('node-fetch') in the resulting bundle so it would still result in a function at run-time while still bundling everything else.

[mhart]

I don't get that:

$ npm install node-fetch
$ npx rollup -c rollup.config.js entry.js -o out-rollup.js
$ node out-rollup.js    
function

Browserify doesn't support ES modules – it's by far one of the oldest bundlers I know and has always been CommonJS AFAIK.

This issue is about using module instead of main when require is called. As it stands, it seems as though esbuild supports bundling Node.js modules, but in reality it's trying to resolve ES modules when you use require, which is just plain incorrect – it's not how require works, I feel we've established that at this point.

I think the question is, does esbuild want to support Node.js' resolution algorithm or not?

[evanw]

I don't get that:

That's interesting. I wonder why we're getting different results with Rollup. The code that Rollup generates looks like this for me:

var lib = /*#__PURE__*/Object.freeze({
    __proto__: null,
    'default': fetch,
    Headers: Headers,
    Request: Request,
    Response: Response,
    FetchError: FetchError
});

console.log(typeof lib);

What does the relevant code that Rollup generates look like for you? Here's a link to an online version that generates the same output.

I think the question is, does esbuild want to support Node.js' resolution algorithm or not?

My goal is to improve esbuild's compatibility both with real-world code and with other bundlers. I do want to make this case work but the fact that other bundlers match esbuild's current behavior gives me pause. Although I can see the argument for having esbuild behave the way you describe since node also behaves that way. You will also get duplicate modules in memory in node if you use both require and import:

• test.mjs:

  import './commonjs.cjs'
  import './esm.mjs'

• common.cjs

  console.log('commonjs', typeof require('node-fetch'))

• esm.mjs

  import fetch from 'node-fetch'
  console.log('esm', typeof fetch)

Running node --experimental-modules test.mjs prints commonjs function and then esm object, so node does load multiple copies of the same module in memory at once. Perhaps esbuild should do this too. It would certainly improve compatibility in this case, but potentially come at the cost of bundle size and possibly the introduction of bugs due to behavior differences between esbuild and other bundlers. I do want to fix this if it's possible but I want to get a complete picture of what other bundlers do first in case any other bundlers have already solved this another way that's better.

[mhart]

But why would I use import and require in my Node.js code? It literally won't run if I use node.

I'm just using require – and I want esbuild to build it just like webpack and browserify and rollup do (yes, some of those need specific settings to prefer main, that's slightly annoying because it's non-obvious, but still, I can do it)

In terms of your rollup output I'm not sure what's going on – you sure you're using the current version of node-fetch?

[evanw]

But why would I use import and require in my Node.js code? It literally won't run if I use node.

Node is adding native support for import. That's why I enabled it using node --experimental-modules above. It's behind a flag now but will become unflagged in the future. You can read more about this in node's documentation.

Even if you don't use both in your code, my concern is that this could still happen when libraries you use do this in their code. So then you end up with multiple copies: the CommonJS version from your code and the ECMAScript module version from the library code.

In terms of your rollup output I'm not sure what's going on – you sure you're using the current version of node-fetch?

I tried both the current version (2.6.1) and the beta version mentioned above (3.0.0-beta.8) and they both resulted in an object when bundled with Rollup. You can see the version information for all of the packages involved here: https://repl.it/@EvanWallace/LinenSwiftAlgorithm#package.json.

[mhart]

the CommonJS version from your code and the ECMAScript module version from the library code

Again, I just don't understand how this would happen if you're just using main to resolve CommonJS modules? Where is the ESM module suddenly getting included from? It certainly doesn't happen in Node.js

[mhart]

You're right on rollup – I completely apologize – it requires configuration. Here's what actually happened:

$ npx rollup -c rollup.config.js entry.js -o out-rollup.js --silent
npx: installed 2 in 0.982s

$ node out-rollup.js 
internal/modules/cjs/loader.js:968
  throw err;
  ^

Error: Cannot find module 'node-fetch'
$ npm install node-fetch

+ node-fetch@2.6.1
added 1 package from 1 contributor and audited 56 packages in 0.764s

$ node out-rollup.js    
function

Forgot to recompile 🤦

It works for node-fetch@next, as with webpack, by specifying main:

import commonjs from '@rollup/plugin-commonjs';
import { nodeResolve } from '@rollup/plugin-node-resolve';
import builtins from 'builtin-modules';

export default {
  output: { exports: 'auto', format: 'cjs' },
  plugins: [
    nodeResolve({ mainFields: ['main'] }),
    commonjs({ ignore: builtins }),
  ],
};

For node-fetch@latest there's the same file-ending ambiguity mentioned earlier and encoding should be ignored:

import commonjs from '@rollup/plugin-commonjs';
import { nodeResolve } from '@rollup/plugin-node-resolve';
import builtins from 'builtin-modules';

export default {
  output: { exports: 'auto', format: 'cjs' },
  plugins: [
    nodeResolve({ mainFields: ['main'], extensions: ['.js'] }),
    commonjs({ ignore: [...builtins, 'encoding'] }),
  ],
};

So in summary:

• Browserify gets Node.js require right out of the box
• Webpack and Rollup can get it right, but they need configuring to use main (and ignore any ES module file extensions)

[mhart]

For completeness, works with ncc too (which I believe uses webpack under the hood?):

$ npx @vercel/ncc run entry.js
npx: installed 1 in 3.066s
ncc: Version 0.24.0
ncc: Compiling file index.js
122kB  sourcemap-register.js
375kB  index.js
190kB  index.js.map
497kB  [913ms] - ncc 0.24.0
function

[evanw]

Ok I'm planning to give fixing this a shot. This is potentially a breaking change so I'm planning to make this when esbuild hits 0.7.0.

[mhart]

Awesome – even if it's just an option allowing the precedence of main vs module, would be great (at least for ppl building specifically for Node.js anyway)

[rtsao]

I'm pretty sure Node.js completely ignores the "module" field of package.json, so it seems more correct to have esbuild ignore it entirely when bundling for --platform=node. This matches the behavior of ncc.

However, given the prevalence of both webpack and "module":, it would not surprise me if there were instances where folks depend on "module" being respected in Node.js bundles. I think it might be worth keeping this behavior around behind a flag to maintain compatibility with webpack defaults.

To be clear, in any case, I would definitely not expect the resolved file (i.e. "main": vs "module":) to differ depending on usage of require vs import.

[evanw]

To be clear, in any case, I would definitely not expect the resolved file (i.e. "main": vs "module":) to differ depending on usage of require vs import.

After doing some research, it appears that a lot of people do expect this. As a counterpoint, here is an example of what kind of problems doing so causes in practice. I think this thread was a good summary of the issue and the opinions of the people behind different bundlers.

One approach I'm currently considering is for require to read from "main" and import to read from "module", but later on during linking when the whole module graph is available, esbuild could rewrite "module" to "main" for any module that is used with both require and import. That should "just work" automatically without any configuration while also not causing problems with duplicate modules in the bundle. One drawback is that esbuild may spend extra time parsing duplicate copies of a module only to throw out that work later, but that seems like an acceptable trade-off to me for being able to handle this case automatically without any configuration.

[rtsao]

Node.js defines a clear mechanism for creating packages that conditionally export different files for require vs import (reproduced below):

{
  "main": "./main-require.cjs",
  "exports": {
    "import": "./main-module.js",
    "require": "./main-require.cjs"
  },
  "type": "module"
}

I would absolutely expect esbuild to respect this, just like Node.js does. However, this is entirely unrelated to "module": which is completely ignored by Node.js.

Regarding module duplication when usage of require and import are mixed, Node.js has some suggestions for package authors on how to preserve singletons.

Broadly speaking, I think there are essentially two overwhelmingly dominant module resolution algorithms used in the JS ecosystem:

1. The vanilla Node.js algorithm (and attempts to replicate this more or less exactly)
2. enhanced-resolve used by webpack, which supports resolving files based on simple precedence of an ordered set of "mainFields" (e.g. ["module", "main]).

I'd fear that adding a third option would merely add further complexity to the already hopelessly complicated module resolution situation.

[evanw]

Keep in mind that this issue is about main and module and that exports is a separate issue: #187. I want to implement that at some point and when that's implemented, packages that use exports shouldn't run into this problem.

I think I largely agree with what you're saying. However, I don't think exports is a valid fix for this issue because there are many dual CJS/ESM packages already in the wild that people are using and will continue to use. Likely many of them will never be updated to use exports and even if they are, other packages will depend on older versions of those packages so the problem will persist.

Part of the design of esbuild is to try to automatically "do the right thing" without too much configuration. All of the little configuration options can add up to quite a bit of overhead. Right now esbuild is pretty simple to configure and use and I'd like to keep that property if possible. If there's a solution that is automatic without configuration and doesn't have significant downsides, then I'm inclined to take that approach.

1. The vanilla Node.js algorithm (and attempts to replicate this more or less exactly)

The node algorithm doesn't appeal to me because you can end up with duplicates of the same module in memory. I understand that it can be worked around but avoiding it entirely seems preferable if possible.

2. enhanced-resolve used by webpack, which supports resolving files based on simple precedence of an ordered set of "mainFields" (e.g. ["module", "main]).

I don't like the idea of the mainFields option because it has a global effect on the whole bundle. It doesn't seem right that having to change mainFields: ["module", "main"] to mainFields: ["main"] to get one package to work correctly has the side effect of disabling tree shaking across the whole bundle, since now no packages are using ESM.

I like the solution I outlined above because it seems to have the best properties of both options. It should automatically handle module.exports = function() {} while still supporting tree shaking if you use import and without causing duplicates of the same module in memory. You can think of it as implementing the mainFields option but the decision is made per package instead of globally.

[kzc]

Is the @material-ui issue described in #81 (comment) an example of this problem?

[mhart]

I'm not sure material-ui is a good example here – all the guidance and tutorials seem to use ES modules. This issue is more for plain-old-Node.js apps using purely require/CommonJS

[rtsao]

Why is it important to have per-package granularity?

If a package is broken by either of the following schemes, it isn't compatible with the vast majority of the ecosystem and is IMHO probably not worth consideration.

1. Always use "main": for Node.js bundles

This is what Node.js, Parcel, ncc, and Next.js do by default. This is the most "correct", but I assume the ability for esbuild to tree shake is harmed. However, is tree-shaking really that crucial for server-side bundles? This seems like a perfectly acceptable outcome to me.

2. Always use "module": for Node.js bundles

This is what Webpack and Rollup (via @rollup/plugin-commonjs and @rollup/plugin-node-resolve) do by default. This is the status quo, and I think this is also acceptable.

Selecting just one option seems adequate, but also having a binary flag between the two would not be a large maintenance burden and conforms to both widely-used methods.

On the other hand, co-opting an existing package.json field (albeit a loosely-defined one) with new semantics runs the risk of having a pernicious influence on the ecosystem. As a user, it is hard enough already to try and understand how bundlers and other tools resolve dependencies. Is yet another way really needed?

I remain puzzled by the notion that "module:" resolution should be somehow predicated on usage of import vs require. The original proposal and its predecessor jsnext:main have nothing to do with this. My understanding is that "module": is and always was based upon bundler capabilities rather than syntax of user code. I think people may be erronesouly conflating conditional export semantics of exports": with "module":. As far as I'm aware, the de facto "module": specification (as defined by implemented by various implementations) doesn't do this.

If there truly is a package that needs special treatment, I think there are plenty of viable workarounds that need not involve a novel module resolution algorithm:

• Marking the package as external
• Using Yarn packageExtensions or patch: protocol to modify the package.json fields as necessary
• Forking the package and/or fixing it upstream
• Using an esbuild resolver plugin

This seems in line with how esbuild handles other non-standard features.

I understand there is sometimes tradeoff between correctness and usability, but in this case, I think it is really worth considering the potential risks of a new module resolution scheme.

Sorry for the long-winded response. You should obviously do what you feel is best for your project (I am merely hoping to persuade you to consider alternatives 😄).

[kzc]

I'm not sure material-ui is a good example here – all the guidance and tutorials seem to use ES modules. This issue is more for plain-old-Node.js apps using purely require/CommonJS

according to #81 (comment):

This switches over from ES6 to CommonJS in UserMenu.js which does a nested import from @material-ui/icons/AccountCircle.js instead of just importing AccountCircle from @material-ui/icons. That bypasses the "module": "./esm/index.js" field @material-ui/icons/package.json and the CommonJS version is picked instead of the ES6 version.

[mhart]

according to #81 (comment):

This switches over from ES6 to CommonJS in UserMenu.js which does a nested import from @material-ui/icons/AccountCircle.js instead of just importing AccountCircle from @material-ui/icons. That bypasses the "module": "./esm/index.js" field @material-ui/icons/package.json and the CommonJS version is picked instead of the ES6 version.

That doesn't preclude it from still not being a good example – as I said, all the guidance there is written under the assumption that if you're using this on Node.js, you're using a bundler. See https://material-ui.com/guides/server-rendering/ . It's a frontend library written with ES in mind – not a plain-old-Node.js app.

[kzc]

I was under the impression that this situation is not specific to --platform=node and could occur anytime the same module is required/imported by different code leading to two copies of the library in memory - commonjs and esm. Anyway, material-ui aside, the proposal in the last paragraph in #363 (comment) seems to be a reasonable solution to avoid duplication.

[mhart]

I'd prefer this issue not get sidetracked with ES module concerns. It's specifically about environments where only require() is used. There's no way an ES module could (or should) get included in such environments.

Think about it: you're writing a standard (pre-v14) Node.js app that works with no bundler. All the code that you write uses require(), so only CommonJS modules from node_modules will be loaded. And all these CommonJS modules, in turn, will only use require() too. There's no way an ES module or anything with an import could be introduced, so there's no chance for ES modules to exist in memory at all.

[mhart]

If there's some clever way to determine that an ES module could be loaded instead of a CommonJS one, and it provides a noticeable benefit, then obviously that's fine – but I think most people would prefer correctness over bundle size – at least for their Node.js apps

[kzc]

It's specifically about environments where only require() is used.

The proposal in #363 (comment) would work in that scenario as well. What's the downside?

[mhart]

It's specifically about environments where only require() is used.

The proposal in #363 (comment) would work in that scenario as well. What's the downside?

It's totally fine by me – only reason I suggested it be configurable was in case there were ppl relying on the existing behavior

[evanw]

Wow this is definitely the most contentious esbuild issue so far :)

I think I'm going to try to go with my proposal. It would automatically fix this issue (require() returning the wrong thing) without regressing other issues (tree shaking and duplication). The main concern seems to be that it's doing something different than other bundlers, but what other bundlers do is either broken or not ideal in various ways.

I also like this approach because it doesn't involve adding any configuration options, so it's easier to roll back and try something else later if it becomes clear later that there's a better solution.

[evanw]

I actually ended up kind of doing both options. By default esbuild will now prefer the main field when targeting the browser if the module is imported using require(), and will now always prefer the main field when targeting node. Also, this behavior is now configurable via the --main-fields= flag. This is documented in more detail in the release notes. I believe this default behavior is now making the right set of trade-offs.

[mhart]

Amazing! 🙌 Thanks so much for taking the time to talk it through.

Has worked well so far in a bunch of scenarios I've tested.

[mhart]

The only case I've had to manage manually is modules with native bindings. Hard to say if this is really the responsibility of a bundler TBH

Eg, test.js:

const bignum = require('bignum')

const b = bignum('782910138827292261791972728324982')

Bundling and running:

$ ~/github/esbuild/esbuild --bundle --platform=node --outfile=test.bundle.js test.js
node_modules/bindings/bindings.js:94:8: warning: Indirect calls to "require" will not be bundled
      : require;
        ~~~~~~~
1 warning

$ node test.bundle.js
/tmp/test/test.bundle.js:116
    throw err;
    ^

Error: Could not locate the bindings file. Tried:
 → /tmp/test/build/bignum.node
 → /tmp/test/build/Debug/bignum.node
 → /tmp/test/build/Release/bignum.node
 → /tmp/test/out/Debug/bignum.node
 → /tmp/test/Debug/bignum.node
 → /tmp/test/out/Release/bignum.node
 → /tmp/test/Release/bignum.node
 → /tmp/test/build/default/bignum.node
 → /tmp/test/compiled/12.18.3/darwin/x64/bignum.node
 → /tmp/test/addon-build/release/install-root/bignum.node
 → /tmp/test/addon-build/debug/install-root/bignum.node
 → /tmp/test/addon-build/default/install-root/bignum.node
 → /tmp/test/lib/binding/node-v72-darwin-x64/bignum.node
    at bindings (/tmp/test/test.bundle.js:112:11)
    at /tmp/test/test.bundle.js:223:31
    at /tmp/test/test.bundle.js:4:5
    at Object.<anonymous> (/tmp/test/test.bundle.js:569:16)
    at Module._compile (internal/modules/cjs/loader.js:1137:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1157:10)
    at Module.load (internal/modules/cjs/loader.js:985:32)
    at Function.Module._load (internal/modules/cjs/loader.js:878:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47 {
  tries: [
    '/tmp/test/build/bignum.node',
    '/tmp/test/build/Debug/bignum.node',
    '/tmp/test/build/Release/bignum.node',
    '/tmp/test/out/Debug/bignum.node',
    '/tmp/test/Debug/bignum.node',
    '/tmp/test/out/Release/bignum.node',
    '/tmp/test/Release/bignum.node',
    '/tmp/test/build/default/bignum.node',
    '/tmp/test/compiled/12.18.3/darwin/x64/bignum.node',
    '/tmp/test/addon-build/release/install-root/bignum.node',
    '/tmp/test/addon-build/debug/install-root/bignum.node',
    '/tmp/test/addon-build/default/install-root/bignum.node',
    '/tmp/test/lib/binding/node-v72-darwin-x64/bignum.node'
  ]
}

$ find . -name '*.node'
./node_modules/bignum/build/Release/bignum.node

So (in this case at least), it's up to the user to manually relocate the *.node file from the module directory in node_modules to one of the directories searched for by bindings.

I think this is totally acceptable – but just in case users post issues with this error, this is the reason.

FWIW, ncc manages this out of the box, by using some pretty specific tactics for bindings and other native requires:

• Binary require support vercel/ncc#85
• Binary relocations vercel/ncc#93

[mhart]

(or of course, declare all modules with native bindings as external, and provide a node_modules with these modules and all their dependencies installed)

[eduardoboucas]

When switching our bundling mechanism to esbuild, we've hit this exact issue with node-fetch. We have users calling this module as require('node-fetch'), which breaks when switching to esbuild because the return value is an object, not a function.

My understanding of what's happening is the following:

• the module's main export is ambiguous, since lib/index may resolve to lib/index.js or lib/index.mjs, depending on which extension the consumer prefers;
• by default, esbuild prefers .mjs over .js, which effectively makes the main export point to an ESM file;
• esbuild will then import an ESM file as a CommonJS module, resulting in an object with a default property as opposed to the expected value, a function.

Changing resolveExtensions to prefer .js over .mjs solves the issue, but it doesn't feel like the right default behaviour. I'm hoping to find a solution that allows existing code to work without compromising better-compliant modules.

The only thing I can think of is to write a plugin that targets require calls to node-fetch and forces the resolved path to be lib/index.js, leaving alone import calls so that ES Modules use the correct entry point:

const replacerPlugin = {
  name: "node-fetch-handler",
  setup(build) {
    build.onResolve({ filter: /^node-fetch$/ }, ({ kind, path }) => {
      if (kind !== "require-call") {
        return;
      }

      return {
        path: require.resolve(path),
      };
    });
  },
};

Does this feel like a reasonable approach?

Thanks in advance!

[evanw]

Changing resolveExtensions to prefer .js over .mjs solves the issue, but it doesn't feel like the right default behaviour.

That's not necessarily true. I don't think node supports implicit .mjs extensions so you could argue that preferring .js would be more compatible when targeting node.

I'm hoping to find a solution that allows existing code to work without compromising better-compliant modules.

If you're trying to customize the behavior for an individual package, then you will likely need to use a plugin.

Does this feel like a reasonable approach?

Personally I would recommend unconditionally redirecting all imports to lib/index.js regardless of how they were imported. I don't think there's any reason to have two copies of the same in module your bundle.

[eduardoboucas]

That's not necessarily true. I don't think node supports implicit .mjs extensions so you could argue that preferring .js would be more compatible when targeting node.

In this case, would you consider changing the default value of resolveExtensions based on the value of target?

Personally I would recommend unconditionally redirecting all imports to lib/index.js regardless of how they were imported. I don't think there's any reason to have two copies of the same in module your bundle.

If someone uses the module with an ES Modules import, don't we want to serve the file referenced in module if it exists?

[evanw]

In this case, would you consider changing the default value of resolveExtensions based on the value of target?

I'm actually considering just removing them entirely. Given that they can break things, they should probably be opt-in instead of opt-out. I'm also considering forbidding importing an ESM file from a CommonJS file in an upcoming version of esbuild for certain reasons (e.g. confusing import order, incompatible with top-level await) and accidentally resolving to a .mjs file would sometimes mess with that too.

If someone uses the module with an ES Modules import, don't we want to serve the file referenced in module if it exists?

Some libraries aren't designed to have two copies of themselves in the bundle. So even if you're ok with bloating your bundle like that, including a library twice can cause correctness issues in addition to bloat.

[IlyaSemenov]

For anyone landing here for the workaround for node-fetch with esbuild, place a patch-package diff at patches/node-fetch+2.6.1.patch:

--- a/node_modules/node-fetch/package.json
+++ b/node_modules/node-fetch/package.json
@@ -2,7 +2,7 @@
     "name": "node-fetch",
     "version": "2.6.1",
     "description": "A light-weight module that brings window.fetch to node.js",
-    "main": "lib/index",
+    "main": "lib/index.js",
     "browser": "./browser.js",
     "module": "lib/index.mjs",
     "files": [

[guerrerocarlos]

This fixes it:
graphql/graphql-js#2721 (comment)