doc: ES module wrapper guide

[fox1t]

📗 API Reference Docs Problem

• Version: 14.7.0

• Subsystem: ESM

Location

Affected URL(s):

• https://nodejs.org/dist/latest-v12.x/docs/api/esm.html#esm_approach_1_use_an_es_module_wrapper

Description

I was wondering if it would be better to write this example as:

{
  "type": "commonjs",
  "main": "./index.js",
  "exports": {
    "import": "./wrapper.mjs",
    "require": "./index.js"
  }
}

since its current form might be misleading for package maintainers that want to support both ESM and CommonJS contexts. Maybe what happened here could be prevented if the documentation was more clear.

---

• I would like to work on this issue and
  submit a pull request.

[aduh95]

package maintainers that want to support both ESM and CommonJS contexts

What do you mean by that? The current example does support both contexts. I does even support old version of Node.js.

Why do you think your example would be clearer? If anything, using explicit extension (.cjs, .mjs) is arguably clearer than using .js extension IMHO.

[fox1t]

I agree with you that .cjs and .mjs are better. However, this issue is about the fact that setting "type": "module" makes .js files become ESM modules. This could be an issue if CommonJS package maintainer just copy-pastes that property and leaves .js as its CommonJS files extension.

[aduh95]

@fox1t that's a valid point! Maybe we could change only the "type": "module" line and keep the .cjs extension? Anyway, PR are welcome.

 {
-  "type": "module",
+  "type": "commonjs",
  "main": "./index.cjs",
  "exports": {
    "import": "./wrapper.mjs",
    "require": "./index.cjs"
  }
}

[MylesBorins]

FWIW the specific layout here was intentional. If you are using exports node will node utilize package.main. Older versions of node.js that don't support Exports also don't support ESM, so main can be used as a fallback for legacy versions of node. Perhaps it would be better if we names it ./index-legacy.cjs?

In regards to the is-promise article, some of the documentation that currently exists was written specifically in response.

[fox1t]

@MylesBorins I am not sure I fully understand what you said here. I think we can think only about the newer Node.js versions, that support exports prop. I think that older Node.js versions are not affected by any mean by "type": "module" setting: is this true?

What I've understood about native ESM in Node.js, is that "type": "module" makes the .js files behave as ESM. If so I am sure that many of the confusion about this docs derives from the fact that the users (and worst of all, package maintainers) copy that example, replaces .cjs with .js and leaves type as module. From what I understand, this is harmful.

That's why I think that writing

{
  "type": "commonjs",
  "main": "./index.js",
  "exports": {
    "import": "./wrapper.mjs",
    "require": "./index.js"
  }
}

in the doc section where we talk about porting legacy packages to be consumed by both world, is way better. I think that 99% of the packages and package maintainers are using .js for their CommonJS files at this point in time and they will transform them to ESM without even noticing it.

@aduh95 I agree with you that .cjs extension would be fine, but it implies that maintainers read thru all of the documentation abut ESM: I am almost sure that the majority will not do that since they are only trying to figure out how to support ESM context. On the other hand, setting "type": "commonjs" in that snippet will at least prevent them from doing really bad things without noticing it.

Of course, I might be wrong about this topic too and in that case, I am open to understanding how it works and, if needed, to update the docs accordingly!

As final words, @MylesBorins that addition about exports: is perfect and makes clear what is the "public API" of the package.

[MylesBorins]

@fox1t There is no version of Node.js that will utilize main if exports exists that also supports modules, thus having a .js file there is essentially setting the module up to fail in all those environments as the .js file will be ESM.

[fox1t]

@MylesBorins I know and completely agree with you about the fact that there will never be overlaps on main and exports props.
Let's see if I manage to make more clear what I am saying.
I'm going to make an example.
I am a maintainer of a package published to npm that many depend on. I don't have a well made CI/CD pipeline that runs all of my tests in every current LTS version of Node.js.
One day I just wake up and I decide to support native ESM. I go to the docs, copy-paste

{
   ...
  "type": "module", // <---- this is the problem I am pointing out here
  "main": "./index.js", // this line is already present in my package.json
  "exports": {
    "import": "./wrapper.mjs",
    "require": "./index.cjs"
  },
...
}

to my package.json and since my current files have .js extension I just change "require": "./index.cjs" to "require": "./index.js".
Since I copied "type": "module" and I am not aware of what it does, I've just broken the package for all node versions that supports the exports field. (and here I am supposing that all of my public API is in index.js file just for semplicity).

I hope that this clarifies why having "type": "commonjs", would be better in a guide that is used by users that already has a codebase with .js files.

P.S. I see this mistake done many times into the wild, by several developers with different skill levels.

[guybedford]

We could remove the "type" field entirely from the example certainly.

[MylesBorins]

@fox1t apologies for missing the point you were trying to get across. +1 on removing module from the example.

[fox1t]

@MylesBorins no worries! I am happy I've managed to explain what I meant! Said that, where are we going to remove "type": "module",? Just in that example alone or are other places where having that line might be harmful?

[MylesBorins]

@fox1t I think opening a PR with just removing module should be sufficient

{
  "main": "./index.js",
  "exports": {
    "import": "./wrapper.mjs",
    "require": "./index.js"
  }
}

[fox1t]

Ok, I am going to make the PR.

[aduh95]

Closed by ac3049d