Browser field regression

[skeggse]

rollup/rollup-plugin-node-resolve#127 introduced changes that broke our particular use of pkg.browser. We have packages that have a single import when used from node, but separate imports when used in browser modules.

For instance, let's say we have a utils module:

utils/
  package.json
  node.index.js
  dist/
    browser/
      string/
        index.js
  src/
    string/
      index.js

// package.json
{
  "name": "utils",
  "main": "node.index.js",
  "browser": {
    "string/index.js": "dist/browser/string/index.js"
  }
}

// node.index.js
exports.string = require('./src/string');

Which we use in two ways:

// from node (no bundler)
const {string: {interpolate}} = require('utils');

// from browser target (with rollup-plugin-node-resolve, {browser: true})
import {interpolate} from 'utils/string';

Because string/index.js isn't a real file, resolve fails to find it, and resolveId gives a falsy resolved. In 3.0.0, this was different, as it took the id utils/string and tried to resolve it with a few different paths, including utils/string/index.js. It looked that key up in the browser object, and got dist/browser/string/index.js, which exited, so all was well.

I realize this may not be a use-case you want to support (I'm curious to know what tweaks you'd make to the structure of the utils package to achieve the import goals), but it was supported in 3.0.0. To me, this makes rollup/rollup-plugin-node-resolve#127 a major rev, and 3.0.1 a backwards-incompatible change.

Not trying to call anybody out on this - it's a super weird use-case; just pointing out that the use-case exists, and wondering if it's something we want to support vs a use-case that shouldn't be supported in the first place.

[skeggse]

I think a better way to frame this issue is that we'd like to use the browser object to resolve "virtual" modules to actual ones.

For example, we might import 'module-name'; with browser: {'./index.js': 'dist/browser/index.js'} in module-name's package.json, but without module-name/index.js being an actual file. This gives us greater flexibility to customize how import interacts with our module, so we can do things like import 'module-name/submodule' without needing submodule to be a directory in the top-level of the module itself.

[RikkiGibson]

I think I'm affected by this same issue, as downgrading from 3.4.0 to 3.0.0 made my "browser" field suddenly start being respected.

I'm struggling to understand just what you mean about string/index.js not being a real file-- is that because it only exists under dist/browser/string/ and src/string/? But node module resolution normally know how to find it under a src/ folder?

Would love to hear from maintainers about what we can do to resolve this.

[skeggse]

I'm struggling to understand just what you mean about string/index.js not being a real file

What I mean by it not being a real file is that in my case there is no file in the module in question named string/index.js.

But node module resolution normally know how to find it under a src/ folder?

Node module resolution does not know anything about the src folder - if it's loading code from the src folder, it's either being required using a path that references the src folder, or it's specified in the package.json file. Node module resolution will, however, look for an index file inside a directory - so we can specify ./string and load ./string/index.js.

[dima-takoy-zz]

The discussion stopped at Sep, 29. Can be closed now?

[skeggse]

It's still a problem for us.

[benwiley4000]

I might have a similar or same problem? When only browser: true is set (accepting the default of module as the preferred field), it seems that we go and get the "browser + module" bundle, but if I remove module from the mainFields, then it starts just getting the regular main (not browser) bundle.

[skeggse]

re: @mecurc if you're interested in moving this along, it turns out I have a PR that attempts to fix this problem: rollup/rollup-plugin-node-resolve#179. I forgot I had that! I'd love to get a review on it, if anyone in this thread is interested.

[melgenek]

It looks like I have a similar case for the dependency that has a browser override. Example, that does not work https://github.com/melgenek/uuid-typescript-rollup

[StreetStrider]

I got the same issue with isomorphic-ws, this package makes sence only if browser field is used in resolving.

[RikkiGibson]

Almost a year later, I look back and realize I understand your use case @skeggse 😄

It would be really helpful for library authors for this to get fixed. I would like to encourage the maintainers to please take a look at rollup/rollup-plugin-node-resolve#179.

[bterlson]

@melgenek I think that issue is not related. It appears to be due to the typescript plugin resolving ids it shouldn't. With rollup-plugin-typescript2, I don't see such issues.

[bterlson]

@StreetStrider can you provide a repro if your isomorphic-ws issue? It seems to work fine for me.

[StreetStrider]

@bterlson will do.
The first minimal repro seemes to be working properly. However, in my toolchain it is not. Will try to add some meat to make it reproducible.

[StreetStrider]

@bterlson I've tracked the case. It occurs when I use all three mainFields:
{ mainFields: [ 'module', 'main', 'browser' ] }.
Looks like the order in that list is significant. When I change it to:
{ mainFields: [ 'browser', 'module', 'main' ] }
everything works ok.

The doc says:

if this list contains "browser", overrides specified in "pkg.browser"

However, not only presense, but order is significant too.

[TrySound]

If you specify mainFields: [ 'module', 'main'] then browser field is automatically prepended.
https://github.com/rollup/rollup-plugin-node-resolve/blob/master/src/index.js#L72

[StreetStrider]

@TrySound ok, but docs say that this mechanisms are alternative to each other:

either use this option or add "browser" to the "mainfields"

It seemes that docs are not in sync with actual code.

[shellscape]

@StreetStrider would you like to submit a PR for updating the docs?

[StreetStrider]

@shellscape, will do.

[StreetStrider]

@shellscape #138