nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.62k stars 29.6k forks source link

Recommend `node`/`default` conditions instead of `require`/`import` as a solution to the dual package hazard #52174

Open nicolo-ribaudo opened 7 months ago

nicolo-ribaudo commented 7 months ago

Affected URL(s)

https://nodejs.org/api/packages.html#dual-package-hazard

Description of the problem

Publishing packages with dual CommonJS and ESM sources, while has the benefits of supporting both CJS consumers and ESM-only platforms, is known to cause problems because Node.js might load both versions. Example:

package.json foo.cjs foo.mjs
```json { "name": "foo", "exports": { "require": "./foo.cjs", "import": "./foo.mjs" } } ``` ```js exports.object = {}; ``` ```js export const object = {}; ```
package.json bar.js
```json { "name": "bar", "main": "./bar.js" } ``` ```js const foo = require("foo"); exports.object = foo.object; ```
// my app

import { object as fooObj } from "foo";
import { object as barObj } from "bar";

console.log(fooObj === barObj); // false?????

The two suggested solutions boil down to "even when you have an ESM entrypoint, still use only CJS internallly". This solves the dual package hazard, but completely defeats the cross-platform benefits of dual modules.

If foo instead used these export conditions:

{
  "name": "foo",
  "exports": {
    "node": "./foo.cjs",
    "default": "./foo.mjs"
  }
}

Then:

We have been using this node/default pattern in @babel/runtime for a couple years, because we wanted to provide an ESM-only version for browsers while still avoiding the dual-package hazard (@babel/runtime is mostly stateless, but @babel/runtime/helpers/temporalUndefined relies on object identity of an object defined in a separate file).

joyeecheung commented 6 months ago

Looks like a good first issue. Relevant file is https://github.com/nodejs/node/blob/main/doc/api/packages.md

eliphazbouye commented 6 months ago

@joyeecheung , @nicolo-ribaudo this issue is still relevant ?

joyeecheung commented 6 months ago

Yes, IMO the doc can still be improved as suggested in the OP.

mews6 commented 5 months ago

Is there still work to be done here?

joyeecheung commented 2 months ago

In https://github.com/nodejs/node/pull/54648 which adds a new pattern I realize that it just doesn't seem appropriate to document them this way in the API docs. To quote my comments:

  1. It teaches opinionated practices that some consider dangerous
  2. It will soon be obsolete when we unflag --experimental-require-module.
  3. It's difficult to understand a multi-file structure via long texts and snippets in a (rendered) markdown document.

I think they should just be placed in their own example repo with some notes on pros/cons and maybe links to threads of discussions. But anyway API docs is the wrong place for them.

B4nan commented 2 months ago

It will soon be obsolete when we unflag --experimental-require-module.

May I ask if there is some schedule for unflagging this? Given the node 22 LTS is nearby, any chance it would happen with that version already?