Fails to resolve browser substitutions when mapping keys have no file extension

[Gozala]

I am running into issue with package libp2p-websockets@0.15.0 which has browser field with following override:

"browser": {
    "src/listener": "./src/listener.browser.js"
},

And a following line in src/index.js (which is also what is set as main):

const createListener = require('./listener')

I would expect that bundling src/index.js would produce a bundle where src/listener.js is substituted for src/listener.browser.js, however instead it fails (on esbuild@0.8.39):

esbuild --version
0.8.39
esbuild node_modules/libp2p-websockets --bundle --main-fields=browser,main       
 > node_modules/libp2p-websockets/node_modules/ipfs-utils/src/env.js: warning: Define "process.env.NODE_ENV" when bundling for the browser
    12 │ ... && typeof process.env !== 'undefined' && process.env.NODE_ENV === 'test'
       ╵                                              ~~~~~~~~~~~~~~~~~~~~

 > node_modules/libp2p-websockets/src/listener.js: error: Could not resolve "os" (set platform to "node" when building for node)
    4 │ const os = require('os')
      ╵                    ~~~~

 > node_modules/it-ws/server.js: error: Could not resolve "http" (set platform to "node" when building for node)
    3 │ var http = require('http')
      ╵                    ~~~~~~

 > node_modules/it-ws/server.js: error: Could not resolve "https" (set platform to "node" when building for node)
    4 │ var https = require('https')
      ╵                     ~~~~~~~

1 warning and 3 errors

[Gozala]

Further investigation seems to show that updating mapping as follows resolves the problem:

"browser": {
    "src/listener.js": "./src/listener.browser.js"
}

However most other tools seem to work without requiring .js extension at the end. I think it would be great if esbuild did support that as well, as there are quite a few packages where I'm running into this.

[evanw]

Ah, sorry about that. I wasn't aware of this edge case. Unfortunately the only documentation about how browser should work is unmaintained and is too sparse on details to fully implement the feature with. Bug reports like this one are how I learn how this feature is actually supposed to work. So thank you very much for creating this issue (especially with so much detail!).

Here is some more specificity on how this feature works in Webpack (the de facto "specification" for this feature). I'm adding this information here for my own benefit and potentially for the benefit of others who come across this issue in the future. I believe these test cases show that you actually have to run the substitution twice, once before path resolution and once after path resolution:

Case 1: .js

Given the files no-ext.js, no-ext-browser.js, ext.js, and ext-browser.js and this package.json:

{
  "browser": {
    "./no-ext": "./no-ext-browser.js",
    "./ext.js": "./ext-browser.js"
  }
}

You must make the following substitutions:

1. ./no-ext → ✅ ./no-ext-browser.js
2. ./no-ext.js → 🚫 (do not substitute)
3. ./ext → ✅ ./ext-browser.js
4. ./ext.js → ✅ ./ext-browser.js

Case 2: /index.js

Given the files no-ext/index.js, no-ext-browser/index.js, ext/index.js, and ext-browser/index.js and this package.json:

{
  "browser": {
    "./no-ext": "./no-ext-browser/index.js",
    "./ext/index.js": "./ext-browser/index.js"
  }
}

You must make the following substitutions:

1. ./no-ext → ✅ ./no-ext-browser/index.js
2. ./no-ext/index.js → 🚫 (do not substitute)
3. ./ext → ✅ ./ext-browser/index.js
4. ./ext/index.js → ✅ ./ext-browser/index.js

[ljharb]

The de facto spec/implementation for this feature is browserify, not webpack. Every path, LHS and RHS, is fully reified using the node module resolution algorithm before doing any lookups.

in other words, yes, you have to run the substitution/normalization multiple times.

[evanw]

The de facto spec/implementation for this feature is browserify, not webpack.

When I said that, I meant that Webpack is currently the most-used implementation of this feature (via npmtrends.com):

In my opinion it would be better if esbuild works like Webpack instead of like Browserify if there's a divergence in behavior, since people are more likely to expect esbuild to work like Webpack.

Here's an example of behavior divergence. If you provide the following substitution in package.json:

{
  "browser": {
    "./foo/bar": "./foo_bar.js"
  }
}

and then import the path foo/bar without a leading ./, Webpack will not apply the substitution but Browserify will. Here I'm planning on following Webpack's behavior instead of Browserify's behavior.

[ljharb]

Webpack is the newcomer, usage aside, and whatever browserify does is the source of truth. If webpack doesn’t do it, it’s a bug in webpack.

If a bug is filed on webpack, I’ll be happy to push them to fix it - it’s surely an oversight.

[evanw]

It would be great to have this field actually fully specified and to push for the behavior to be consistent across bundlers. However, that seems like a semi-big project and I don't currently have the desire to do that myself. It could be cool if someone else wants to take on that task though.

For whoever does want to take this on: The level of detail needed for the specification to be useful to implementers is something more like node's resolve algorithm specification than the current specification for the browser field. I suspect specifying the browser resolution algorithm as a diff to node's resolution algorithm would be a good way to do it. When specifying this accurately, it's also important to say what happens when multiple parent directories have package.json files with browser fields (I'm still not sure what's supposed to happen there). And ideally this would all have an actively-maintained test suite that includes lots of edge cases to verify that everyone implements it the same way. It's a lot of work :)

The behavior divergence I mentioned previously seems like a bug in Browserify to me, not a bug in Webpack. There is nothing actually at the path ./foo/bar in that example since foo/bar is a package path import. The relative path version would be something like ../foo/bar or ./node_modules/foo/bar depending on where the code is in the node_modules hierarchy. There is already a bug report for this but it was filed against the specification, which was then ignored because the specification is unmaintained: https://github.com/defunctzombie/package-browser-field-spec/issues/13. FWIW there are also other Browserify behavior differences with browser handling vs. Webpack such as https://github.com/browserify/browserify/issues/1903 which would also need to be resolved when coming up with an accurate specification.

Edit: I filed this behavior divergence as a Browserify bug here: https://github.com/browserify/browserify/issues/1994.

[evanw]

Even if the file extension issue is fixed, the package mentioned in the original issue still doesn't work because it's also missing the leading ./. I did some more tests and it appears that this is an area where Webpack behaves differently than both Browserify and Parcel: https://github.com/evanw/package.json-browser-tests. So it's less clear to me if this could be considered a bug in Webpack or not.

@ljharb I'll give you an opportunity to pursue this with Webpack and/or Browserify if you'd like. So I will hold off on making this change in esbuild for the moment. However, if this is ends up remaining unresolved, I am currently thinking about changing esbuild to match Webpack's behavior here.

[ljharb]

The leading ./ is a long-time webpack bug in main, and simply isn’t required by node or browserify - so i assume there’s a similar bug with the browser field.

[ljharb]

ftr the package I’m representing here is https://github.com/es-shims/globalThis which works just fine with both webpack and browserify, and has a leading ./

[evanw]

Ok, I gave up and made some tests after all: https://github.com/evanw/package-json-browser-tests. Each test works in one of the bundlers, but there is no bundler that works with all of the tests. This is a mess.

I assume a good end state is for esbuild to support the union of all of the behaviors of all of the other bundlers, so packages using this feature might break in other bundlers but they won't break in esbuild.

[ljharb]

Indeed, the lack of such tests in the first place has caused a lot of deviation. Thanks for making this; hopefully it can be used to bring all bundlers into compliance with the same feature set (or make it clear which things aren’t valid, like a number of the webpack-only ones).