doc: fixes ESM/CJS wrapper example

[fox1t]

This PR updates ESM wrapper example to prevent package maintainers to broke packages after copy-pasting.

Ref: #34714

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[mcollina]

lgtm

[ljharb]

the CJS extension was an intentional choice here; what would be broken without this change?

[jasnell]

Removed fast-track label given @ljharb's comment

[fox1t]

@ljharb here, a real world example: #34714 (comment)

[ljharb]

That discusses only removing type module, not changing extensions.

[fox1t]

@MylesBorins make a snippet and I copied it: #34714 (comment)
How ever if we decide to use .cjs I can update the PR.

[ljharb]

I think it's useful to leave the .cjs there so people know it exists.

Separately, by using .cjs, type: "module" doesn't actually have any effect, so i'm still a bit confused by what's broken.

[fox1t]

Because what the maintainers do is to copy-paste that snippet and since they already have all the codebase that has .js extension, just removes the c letter in both places. Since type is module (and they don't know that leaving it like that makes the js files be interpreted as ESM) they brake the package in Node versions that support type module property (so the newer ones). As I pointed out, the snippet is fine but ESM is still new and developers doesn't know how to deal with it.

[GeoffreyBooth]

This is not an improvement. The example was written intentionally, as we want to encourage people to include the "type" field, even if they also use explicit extensions (as in this example).

[fox1t]

@GeoffreyBooth I tend to agree with you. However, I think that in this transition period it is important to be sure that the package maintainers are not going to change their pacakge.json file to:

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

Any ideas? We can add a warning note that points out that "type": "module", will make the legacy .js files act as ESM?

[GeoffreyBooth]

We can add a warning note that points out that "type": "module", will make the legacy .js files act as ESM?

The example doesn't use .js at all, so a warning seems unnecessary.

[fox1t]

@GeoffreyBooth, the purpose of this snippet example is to make it easy to support ESM in CommonJS code. Its audience is developers that have CJS files all over the place (with .js extension). They are implicitly using "type": "commonjs" today, without even knowing it. I think that putting both "type": "module" and .cjs in that example could lead to really bad situations that I tried to explain as best as I could.

In addition to that, I hope that we all agree that the scope of a guide/documentation is to be easily understandable by just looking at the paragraph that you need. We cannot hope that all of the devs that are interested in supporting ESM in "legacy" packages are going to read all of the documentation about ESM in order to learn that they have to put "type": "commonjs", if they are NOT going to change all of their files to have .cjs extension.

[GeoffreyBooth]

It sounds like your concern is that people will copy this example, change the .cjs or .mjs to .js as appropriate for their codebase, and move on without realizing the consequences of using the .js extension. Is that about right?

So perhaps what would satisfy that concern would be a sentence below the snippet like:

Note that the above example uses explicit extensions .mjs and .cjs. If your files use the .js extension, note that "type": "module" will cause such files to be treated as ES modules, just as "type": "commonjs" would cause them to be treated as CommonJS. See Enabling.

[fox1t]

@GeoffreyBooth I think that this might fix this specific problem. I am going to revert the snippet and add the disclaimer at the bottom of it. Is it fine?

[ljharb]

LGTM!

[GeoffreyBooth]

You need to change the link to Enabling to be relative to the page (see the other links) but otherwise this is fine.