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: esm: options for package authors #29497

Closed
wants to merge 4 commits into from

Conversation

@GeoffreyBooth
Copy link
Contributor

commented Sep 8, 2019

This updates the ECMAScript Modules docs with a few changes agreed in recent modules group meetings:

  • Per the 2019-06-19 meeting, we decided that we want to encourage all package authors to always include the package.json "type" field, even for CommonJS packages (where "type": "commonjs" is implied).

  • Per the 2019-08-28 meeting, we decided that the docs needed more discussion of packages and specifically what options are available for package authors wishing to support both CommonJS and ESM consumers of packages, and both current and older versions of Node.

Regarding the latter, this PR is just a first step; it will need expansion if/when #29494 is merged in, and possibly again later if more improvements are made to better support different module systems or Node runtime versions. For now, though, I think this text is an improvement on the very minimal mention that dual CommonJS/ESM support in packages has currently.

Checklist

//cc @nodejs/modules-active-members

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved

@GeoffreyBooth GeoffreyBooth force-pushed the GeoffreyBooth:esm-package-doc branch from aaeceba to c28ee9b Sep 9, 2019

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Does this PR need anything else? @Trott?

I’d like to get this merged in sooner than later because #29494 will make further changes to the ESM docs, and it would be good to get this in before that.

doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
@Trott

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@Trott

This comment has been minimized.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Anyone care to review? 🙋

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@Trott
Trott approved these changes Sep 16, 2019
Copy link
Member

left a comment

I'm not a subject-matter expert on ES modules by any means, but LGTM other than needing to move a bottom reference to a different location in the list.

I evaluated it it for formatting and clarity of text. It would be great if someone from @nodejs/modules-active-members could sign off on it (or provide a brief explanation of why perhaps it shouldn't land).

@Trott Trott added the author ready label Sep 16, 2019

@jkrems
jkrems approved these changes Sep 16, 2019
@Trott

This comment has been minimized.

@WebReflection

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

not sure if late for this party, but as soon as you decide to reserve an entry in package.json, maybe it makes more sense to make it flexible, able to scale.

"type": "commonjs" limits the scope of the type property to a single type, which might be fine, but how about using type as an object instead, where multiple types are meant?

{
  // backward compatible
  "main": "cjs/index.js",
  "type": {
    "commonjs": "cjs/index.js",
    "esm": "esm/index.mjs",
    "json": "json/index.json",
    "wasm": "wasm/index.wasm"
  }
}

Something like that would help dual, triple, quadruple package modules that could include present and future of any format we could come up with.

TL;DR why would type be limited to a single use case, where in a transition phase, and in a multi-module publishing, we could have all pointers in that very same place?

@ljharb

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

@WebReflection sounds like you're asking for nodejs/modules#283 :-)

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

not sure if late for this party, but as soon as you decide to reserve an entry in package.json, maybe it makes more sense to make it flexible, able to scale.

This is outside the scope of this PR, but it’s under development in nodejs/modules#368.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

I don't think type is a meaningful differentiator there. I don't think "let people load .wasm as image/png!" is a use case node needs to support. .js is - afaik - the only truly ambiguous file extension that has different interpretations inside of node.

@WebReflection

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@ljharb @GeoffreyBooth thanks, not sure it's the same, but I'll have a look

@jkrems

.js is - afaik - the only truly ambiguous file

what I've said has nothing to do with .js, it's about the module.

A dual module would like to point at both CJS and ESM, but if loaded via WASM (in the future) it could point at WASM too, whenever it'll get some module namespace.

Having a type field that describes a single module type is limiting already for dual packages modules, so that having an entry that would scale any scenario would be, IMHO, wiser at this point, as every trouble we had so far was because we never thought .js or, in general, an npm module, could be anything different than CJS, so that avoiding the same mistake now, for any future iteration of NodeJS modules, would be ideal.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

I'm not sure what you mean by "loaded as WASM" - wouldn't you expect that if the same package is reachable from JS and WASM, it would do the same thing / point to the same thing? In my head those boundaries between modules shouldn't care that either side is WASM or JS..? I would expect that if I have gone through the pains of shipping a binary AST format in the future, it would always be used even if the importing module happens to be written in plain JS.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@WebReflection please open an issue in https://github.com/nodejs/modules if you don’t mind, this PR is just a documentation update 😄

@WebReflection

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

I would expect that if I have gone through the pains of shipping a binary AST format in the future, it would always be used even if the importing module happens to be written in plain JS.

if the bundler doesn't understand that, it can fallback.
if the WASM module loader wouldn't find the WASM, it can fallback.

and so on and so forth, there are not just two kind of modules, as there was never just one kind, hence my comment.

@GeoffreyBooth if you think it's needed, I will.

Regaards

@Trott Trott force-pushed the nodejs:master branch from 1ecc406 to 49cf67e Sep 17, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

As ES Modules are Experimental and therefore subject to any and all changes in any version for the time being, I'm going to go ahead and land this documentation update. Changes to type (and anything else in ES Modules in Node.js) are totally fair game and we'll just update the docs again if/when those changes happen.

Trott added a commit that referenced this pull request Sep 17, 2019
doc: explain esm options for package authors
* organize sections more hierarchically

* recommend always including "type" field, per 2019-06-19 meeting

Refs: nodejs/modules#342 (comment)

* expand discussion of publishing cjs/esm packages

PR-URL: #29497
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

Landed in 80cc6f5

@Trott Trott closed this Sep 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.