node-resolve: “browser” field not being used for sub imports in dependencies

[mikeal]

• Rollup Plugin Name: node-resolve
• Rollup Plugin Version: 11.1.1
• Rollup Version: 2.38.1
• Operating System (or Browser): linux
• Node Version: 14.x
• Link to reproduction (⚠️ read below):

This can’t be reproduced using the above guidelines because it requires a dependency be installed via Node.js that is importing a sub export in an export map that has both “import” and “browser” fields.

The issue is simple to explain. I have a dependency installed that is written in native ESM. One of the exports in the export
map has an import and a browser field. If the plugin applied the same logic as the main field then I would get back the browser entrypoint w/ { browser: true } configured. But I don’t, I get back the import target which is intended for Node.js.

I’ve tracked the issue all the way down and it’s here https://github.com/rollup/plugins/blob/master/packages/node-resolve/src/package/resolvePackageTarget.js#L88-L105

  if (target && typeof target === 'object') {
    for (const [key, value] of Object.entries(target)) {
      if (key === 'default' || context.conditions.includes(key)) {
        const resolved = await resolvePackageTarget(context, {
          target: value,
          subpath,
          pattern,
          internal
        });

        // return if defined or null, but not undefined
        if (resolved !== undefined) {
          return resolved;
        }
      }
    }
    return undefined;
  }

A few problems:

1. “browser” is never added by this plugin to context.conditions. I can work around this locally by adding a config setting of { exportConditions: [ “browser” ] } but that still leaves me with the second problem.

2. This resolution logic is applied in the order it is defined in package.json rather than the order defined by the conditions. This is really not what you want, you want to configure the preferred ordering in the compiler and treat the export map as unordered. It’s just generally a bad idea to rely on map ordering.

While I can tell you for certain that this is where the bug is, I’m not sure this is where you want to fix it. This module is rather complicated and there’s a lot of userBrowserOverride logic in other places that just isn’t applied when you’re importing something other than the main field.

Expected Behavior

Read above.

Actual Behavior

Read above.

Additional Information

This complicated issue template is a big barrier to entry for logging issues.

[LarsDenBakker]

Rollup works in the browser and in node, so it cant add the browser condition by default mode.

Using the order of the keys in the pkg json is how node js specced this, we are following that. Otherwise this becomes inconsistent between tools.

[mikeal]

It doesn’t add the browser condition on { browser: true } though. I use rollup for Node.js compiles so yes, please don’t make this default, but it should resolve sub imports from export maps without adding exportConditions.

Can you point me to the spec that says browser field is secondary when placed later in the map? This is a real problem for systems that produce deterministic maps so I’ll need to go over there and explain this issue.

[LarsDenBakker]

That's true... the browser option opts into resolving using the custom browser field in the package.json. It's a system separate from the package entrypoints system. We could make the option also set the browser export condition, but that might make it confusing for users as well.

The full resolve algorithm is here: https://nodejs.org/dist/latest-v15.x/docs/api/esm.html#esm_resolver_algorithm_specification

[lukeed]

Ran into this too. The same mainFields + browser configuration should apply at every level of package resolution.
The gist of what's happening inside the issue I've linked:

• import PackageA (follows browser correctly)
• PackageA imports PackageB (does not follow browser at all)

Both were setup for Node.js by default, but both also have browser and exports['.'].browser fields setup correctly.

[GeorgeTailor]

Same problem when using https://www.npmjs.com/package/uuid and trying to build a package for nodejs env. Apparently, this plugin pulls code for window.crypto instead of nodejs' crypto

[guybedford]

With #866, when setting browser: true "browser" will automatically be added as an exports condition now resolving the first item here.

In a subsequent fix the conditions were matched based on object order as well so this should be working correctly now. Please post a new issue if there are any further inconsistencies, but I believe we should be to spec now.