Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: fixes ESM/CJS wrapper example #34853

Closed
wants to merge 5 commits into from
Closed

doc: fixes ESM/CJS wrapper example #34853

wants to merge 5 commits into from

Conversation

fox1t
Copy link
Contributor

@fox1t fox1t commented Aug 20, 2020

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 nodejs-github-bot added doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. labels Aug 20, 2020
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jasnell jasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 20, 2020
@ljharb
Copy link
Member

ljharb commented Aug 20, 2020

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

@jasnell

This comment has been minimized.

@jasnell jasnell removed the fast-track PRs that do not need to wait for 48 hours to land. label Aug 20, 2020
@jasnell
Copy link
Member

jasnell commented Aug 20, 2020

Removed fast-track label given @ljharb's comment

@fox1t
Copy link
Contributor Author

fox1t commented Aug 20, 2020

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

@ljharb
Copy link
Member

ljharb commented Aug 20, 2020

That discusses only removing type module, not changing extensions.

@fox1t
Copy link
Contributor Author

fox1t commented Aug 20, 2020

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

@ljharb
Copy link
Member

ljharb commented Aug 20, 2020

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
Copy link
Contributor Author

fox1t commented Aug 20, 2020

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.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

fox1t commented Aug 20, 2020

@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
Copy link
Member

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
Copy link
Contributor Author

fox1t commented Aug 20, 2020

@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
Copy link
Member

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
Copy link
Contributor Author

fox1t commented Aug 21, 2020

@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?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34853
Refs: #34714
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34853
Refs: #34714
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants