[esinstall] Package Export keys should be used in-order

[developit]

The spec indicates that the ordering of keys in "exports" objects defines their precedence, where the first matching key (direct string or via object traversal to matched nested key) is used. Currently, esinstall uses its configured keys (packageExportLookupFields) as the precedence ordering: https://github.com/snowpackjs/snowpack/blob/b00d0cf08ce9c04d18a05a0592c4cdb5cf2477c4/esinstall/src/entrypoints.ts#L96-L103

This causes the following export map to be interpreted incorrectly:

{
  "exports": {
    "import": "./dist/index.mjs",
    "browser": "./dist/browser.js",
    "default": "./dist/fallback.umd.js"
  }
}

Using the above package with packageExportLookupFields:['browser'] will resolve an import statement to ./dist/browser.js rather than the expected ./dist/index.mjs.

[FredKSchott]

interesting! We rely on rollup for this feature, and just pass that property to Rollup. Here's the PR where that was implemented in Rollup with the erroneous behavior that you're describing: rollup/plugins#540

/cc @LarsDenBakker

[LarsDenBakker]

Interesting.. I did actually re-implement the whole thing in rollup/plugins#693 where the implementation follows the spec more literally. Looks like it follows the exports order instead of the conditions order: https://github.com/rollup/plugins/blob/master/packages/node-resolve/src/package/resolvePackageTarget.js#L90

It's merged, but hasn't been released yet.

[developit]

Is the code I linked to in esinstall unused then?

I'm also curious if esinstall/snowpack currently support the browser export map key.

[FredKSchott]

Ah you're right, it is unused. Two things missing:

1. We should be passing packageExportLookupFields to that function
2. We should be implementing the lookup using the correct lookup order that you mentioned

[developit]

If it's helpful, Evan implemented the same change in Vite just now: vitejs/vite#1418 (comment)

[FredKSchott]

Also note that that logic you linked to isn't yet released to latest and only exists on next. Will go out to latest with Snowpack v3.0 next week.

[FredKSchott]

appreciate the link!

[drwpow]

Using the above package with packageExportLookupFields:['browser'] will resolve an import statement to ./dist/browser.js rather than the expected ./dist/index.mjs.

This is correct Node behavior as you said, but one question I have is: from a community standpoint, there are packages where both are used: import is used for ESM Node, and browser is used for Browser-compatible code. Also, according to the docs:

Other conditions such as "browser", "electron", "deno", "react-native", etc., are unknown to Node.js, and thus ignored.

Knowing that a package may have both browser and import conditional exports, "browser" can be used without affecting Node, and Snowpack/esinstall are building for the browser, shouldn’t browser be preferred only in Snowpack’s case? Happy for pushback; just throwing this scenario out there.

[developit]

@drwpow supporting "browser" is totally up to Snowpack to decide, but implementations should not give precedence to a given field arbitrarily. Instead, module authors are the ones who decide whether the "browser" field should be given precedence over "import", which they can do by changing the order.

Here's a package.json that tells the bundler/runtime "use my browser build when bundling for the browser":

{
  "exports": {
    "browser": "./browser-modern.mjs",
    "import": "./unknown.mjs",
    "default": "./fallback.js"
  }
}

Here's a package.json that tells the bundler/runtime "use my browser build for import statements when bundling for the browser, otherwise fall back to my ESM build or UMD bundle":

{
  "exports": {
    "browser": {
      "import": "./browser-modern.mjs"
    },
    "import": "./unknown.mjs",
    "default": "./fallback.umd.js"
  }
}

However, some packages ship universal (not browser-specific) "import" entry modules. With key-order based precedence, that ends up being pretty straightforward too:

{
  "exports": {
    "import": "./universal-modern.mjs",
    "browser": "./browser.umd.js",
    "default": "./universal.umd.js"
  }
}

This last pattern is particularly important given the introduction of package imports, which allow re-mapping specific internal modules within package (such as to apply browser/node variants):

{
  "exports": {
    "import": "./modern.mjs",   // includes `import fetch from 'fetch'`
    "default": "./fallback.js"   // includes `fetch = require('fetch')`
  },
  "imports": {
    "#fetch": {
      "browser": "./native-fetch.mjs",
      "default": "node-fetch",
    }
  },
  "peerDependencies": {
    "node-fetch": "*"
  }
}

[drwpow]

Instead, module authors are the ones who decide whether the "browser" field should be given precedence over "import", which they can do by changing the order.

Totally fair! Thanks for clarifying that point. I’m on board with that.

[developit]

Happy to! I keep meaning to write a legit guide on this, I feel like the ecosystem could really use something. It's these little details that end up making "exports" more viable and as a tool to move things forward.

To add a bit of context to the above: I know it's a little concerning to go with Node's solution where the field precedence decision is in the hands of package authors. But consider: the alternative (custom or user-defined precedence) is actually the same as the solution we had before (package main fields). Without this consistency, Package Exports are just a nested version of the existing the pkg.browser / pkg.main / pkg.module fields.

[FredKSchott]

Trusting package authors, Jason??? 🙀

[matthewp]

Key ordering is an odd API choice and I worry that authors are not going to realize it and make mistakes. You can also imagine a scenario where runtimes support the same conditions and so you can never target each runtime with different modules. For ex. if Deno ever supported this and you wanted to use different entrypoints for Deno and Node, you couldn't really do that.

[matthewp]

@developit in your original example:

{
  "exports": {
    "import": "./dist/index.mjs",
    "browser": "./dist/browser.js",
    "default": "./dist/fallback.umd.js"
  }
}

You want the bundler to include import, then what is browser for?

To rephrase, is there a tool that understands export maps and the browser field but doesn't support import as well? (Same question for your UMD default)? Just wondering when you'd ever use anything other than browser, node, duktape, etc

[FredKSchott]

I do agree that it assumes an understanding of export maps that I just haven't seen so far in the ecosystem. For example, I've spent plenty of time looking at export maps and didn't realize this behavior until you brought it up. Definitely a little concerned about this path forward, even if it is The Right Thing to do.

[developit]

@matthewp in that example, browser would be used by something like unpkg. Here's a clearer example:

{
  "exports": {
    "import": "./dist/index.mjs",
    "browser": {
      "require": "./dist/browser.umd.js"
    },
    "default": "./dist/fallback.umd.js"
  }
}

[developit]

The understanding piece is definitely and important one. For me though, the lightbulb moment was realizing that we already tried custom semantics and know those erode over time as a result of misuse, whereas package-defined semantics can evolve over time. Poor ordering choices get filed as bugs in the offending package, which is something the package author can then address. One of those "distributed problem requires distributed solution" type things.

[matthewp]

@developit Does unpkg support exports.browser? As far as I can tell it doesn't read the exports map at all currently: https://github.com/mjackson/unpkg/blob/9f44cbf0135ede5201115449debf3b7a34538921/modules/middleware/validateFilename.js

Let's pretend it did support it though, presumably it would also support exports.import. In which case, when would browser ever be used? Are you imagining that it perhaps browser sniffs and determines an ES5 browser and therefore skips over import? But we are seeing that browser is used (contrary to your example) for ESM code, so I imagine it would want to skip over browser as well.

So it seems like the only reason to have import and browser together is if browser comes first, perhaps because import is not compatible with the browser.

[matthewp]

btw, I'm not arguing we shouldn't support the ordering, I think we should until there's a more serious problem, just pointing out that import before browser is almost definitely a mistake (unless there's a case I'm not thinking of) and I wonder if there's a way to surface this some how...

[developit]

Unpkg doesn't support the exports field at all, no. I was using it as an example. Another example of this would be when bundling with Webpack or Rollup while targeting UMD.

The reason this might seem superfluous is because bundlers are most commonly used to produce browser code - but that association is not actually a generalizable one, it's just a common case.

Ultimately, the use-case for being able to give "browser" precedence over "import" is when packages have browser-specific behavior. There isn't actually a reason to assume "browser" implies ESM usage - environments like Electron are examples where one might actually want to provide a CJS version of a browser-specific entry. UMD is another such example:

{
  "name": "isomorphic-fetch",
  "exports": {
    "browser": {
      "import": "./browser.mjs",  // "export default fetch"
      "default": "./browser.umd.js",  // "(function(g,f){g.fetch=f();...})(this,function(e){e.fetch=function(){...}})"
    },
    "import": "./polyfill.mjs"
    "default": "./polyfill.js"
  }
}

Another way to consider this that might help clarify the need for author control over ordering is to consider what happens when user-defined Package Export fields are configured. The package author needs to be able to specify that "production" takes precedence over "browser", regardless of what the bundler configuration says.

[AJamesPhillips]

I'm not sure if this is related but we upgrade to snowpack 3 recently and the import order of some @material-ui/core css assets changed which broke their menu styling. We haven't managed to get a reproducible demo of the bug but would this be of interest to snowpack / rollup if we could or would this be a material-ui thing?

Specifically the order of these two items seems to randomly change in our app (this is the attempt to reproduce so it's got a lot less going on)