Consider removing module to better support Rollup + ESM

[patrickarlt]

Hello there I ran into an issue when testing a library that I author (@esri/arcgis-rest-request) includes a dependency on formdata-node and then bundling an application that includes that library as a dependency with Rollup.

According to https://github.com/rollup/rollup/issues/3514#issuecomment-620548059:

My feeling is that by default, Rollup should check module, then ESM exports, then other alternatives.

This means that by default Rollup will pickup "module": "./lib/esm/index.js", which is the Node version of formdata-node. If the Rollup config uses the browser option of @rollup/plugin-node-resolve then this will pickup the "browser": "./lib/cjs/browser.js", which is Common JS and will require users to add @rollup/plugin-commonjs.

I don't think there is a good way to get Rollup to work without removing the main, module and browser fields. Then Rollup will pickup exports and get to the correct import or require for the browser.

As an easy workaround I did find that I can add formdata-node to the globals and externals and it just points back at the browsers global FormData:

// rollup.config.js
import nodeResolve from "@rollup/plugin-node-resolve";

export default {
  input: "src/index.js",
  output: {
    file: "dist/bundle.js",
    format: "iife",
    globals: {
      "formdata-node": "globalThis", // Makes this look at `globalThis.FormData, i.e. native browser FormData`
    },
  },
  external: ["formdata-node"], // Mark as an external module not to be included in the bundle

  plugins: [
    nodeResolve()
  ],
};

I'm not sure if you would consider removing the main/module/browser fields or not. I understand these exist for legacy clients/bundlers, ect... If you aren't would you consider a PR discussing this in the docs somewhere? I would be happy to write one.

[octet-stream]

I'm not sure if you would consider removing the main/module/browser fields or not. I understand these exist for legacy clients/bundlers, ect...

That's correct. These fields might be necessary for older environments. Not sure about module tho, it seem to be not necessary in a future (because it's not a standard) and be replaced with export which is already supported in Node.js v12.20 which is required for formdata-node usage (because of conditional export support). I'm not so sure how different bundlers resolve browser and module field and whether I can drop both already or should keep it there. I do care less about bundlers and more about Node.js itself, because this it is the main target for this module. Perhaps it needed to be discussed further, before we decide what todo.

---

However I plan to drop both main and module at least in the next major release, because for Node exports will be enough to tell the module's entry point.

[patrickarlt]

Thanks for the response. I'll test again after the next major update.

I do care less about bundlers and more about Node.js itself,

This is pretty valid attitude 😄 If I run into issues I can always warp formdata-node in my own package to work around anything.

Thanks for a great package!

[octet-stream]

I'll test again after the next major update.

It will be released around Node.js v12 EOL, btw

This is pretty valid attitude 😄 If I run into issues I can always warp formdata-node in my own package to work around anything.

I just meant that my main target is Node, and for Node I can already get rid of main, module, and bwrowser fields because I expect people to use with with at least Node.js 12.20 which have support for both export field and conditional exports. But in case of bundlers - there's just too many of them and they seem to handle these fields differently. For example, webpack will look for export first, and then for anything else, which means for latest version of webpack I can keep export and it should be okay. However, if we can find any solution to this, that won't break anything - I will accept it. Any helpful feedback or PR is always welcome here.

[patrickarlt]

Ok I figured this out. Rollup looks for module then exports then main by default. It was finding https://github.com/octet-stream/form-data/blob/master/package.json#L17 which defines the NODE ESM entry point which causes rollup to try to bundle everything the Node version.

"module": "./lib/esm/index.js",

I ended up solving this by wrapping formdata-node in a package.json that looked like:

{
  "name": "@esri/arcgis-rest-form-data",
  "version": "3.3.0",
  "module": "browser-ponyfill.mjs",
  "browser": "browser-ponyfill.js",
  "exports": {
    "module": "./browser-ponyfill.mjs",
    "browser": "./browser-ponyfill.js",
    "import": "./node-ponyfill.mjs",
    "require": "./node-ponyfill.js",
    "default": "./browser-ponyfill.mjs"
  },
  "types": "index.types.d.ts",
  "type": "commonjs",
  "dependencies": {
    "formdata-node": "^4.1.0"
  }
}

Which works for my use case.

I think the community convention from bundlers is that module should be an ESM module for the browser, not for Node. This seems true least in Parcel v2+ https://v2.parceljs.org/features/dependency-resolution/#package-entries, Rollup https://github.com/rollup/plugins/tree/master/packages/node-resolve/#mainfields and Webpack https://webpack.js.org/configuration/resolve/#resolvemainfields.

Interestingly Webpack works fine because it prefers exports so it finds exports > browser > import and works.

If you are interested in maximizing compatibility with bundlers in the next major version I think that you should leave module in but point it to ./lib/esm/brower.js and just get rid of main.

[octet-stream]

Interestingly Webpack works fine because it prefers exports so it finds exports > browser > import and works.

Node.js will also look up at the exports field first. For my module, the resolution will look like: exports > node > import for ESM and exports > node > require for CJS. Maybe I misunderstood the purpose of the module field? Maybe I need to find some more clarification on that topic.

See: https://nodejs.org/dist/latest/docs/api/packages.html#packages_package_entry_points The documentation says:

If both "exports" and "main" are defined, the "exports" field takes precedence over "main". "exports" are not specific to ES modules or CommonJS; "main" is overridden by "exports" if it exists. As such "main" cannot be used as a fallback for CommonJS but it can be used as a fallback for legacy versions of Node.js that do not support the "exports" field.

[octet-stream]

Also, I can't find anything about module field in Node.js documentation.

[octet-stream]

I suppose since I'm targeting Node.js v12.20 which is have export field, changing or removing the module field shouldn't really break anything... At least Node.js and Webpack will look up for the export first.

[patrickarlt]

Also, I can't find anything about module field in Node.js documentation.

Digging in more there are a bunch of links pointing to the old Node GitHub wiki. If you git clone https://github.com/nodejs/node.wiki.git and look at ES6-Module-Detection-in-Node.md under "Option 4":

Option 4a: Single Module Entry Point (REJECTED)

Details: package.json has a "main" for CJS Module entry and "module" for ES6 Module entry.

{
  // ...
  "module": "lib/index.js",
  "main": "old/index.js",
  // ...
}

Note: "module" seems like a better option than our previous proposal "jsnext:main", which is being used today in many modules.

Pros:

• package.json pros
• Harmonizes with <script type="module">
• Allows simultaneous new and old entry point
• Replaces "main" (which eventually becomes "the old way") with an ergonomic standard way

Cons:

• package.json cons
• Requires external method for specifying Module System of non-entry points, e.g. require('lib/something-else.js') - which kind of defeats the purpose of this option. See other options under 4 that specify this. (cause of rejection)

I think this was abandoned as the approach and then I'm guessing bundlers picked it up since?

[patrickarlt]

changing or removing the module field shouldn't really break anything...

Removing it or leaving it as is would break Rollup because Rollup doesn't look at the browser field by default. Users would have to enable the browser option of @rollup/plugin-node-resolve to get it to lookup exports > browser > import.

I'm actually indifferent now since it looks like the community never really got togather and standardized module it just sort of happened because of Webpack and Rollup but it is confusing since it WAS part of the Node JS proposal for ES modules.

[octet-stream]

Removing it or leaving it as is would break Rollup because Rollup doesn't look at the browser field by default.

I was talking about Node.js :) Because it will look at exports and never use module (and it doesn't seem to care about this field), we can use it to make it work with Rollup and other bundlers, if there's any of those that are use this field in their module resolution algorithm.

[octet-stream]

I have updated the module field so it will point to browser entry point.

[octet-stream]

I should probably release this change as a minor update, just in case...

[octet-stream]

You can try now the https://github.com/octet-stream/form-data/releases/tag/v4.2.0

[patrickarlt]

Works great thanks!

[octet-stream]

Nice. I am closing this then.