-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: add package.json documentation #32970
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution :) I like this change, I think it makes sense.
@nodejs/modules this significantly refactors the documentation for ESM PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the overall idea of this, I'm putting a block on it only to ensure this gets adequate review before this lands. This is a big change and I want to make sure we give it some thorough review.
doc/api/esm.md
Outdated
### Package Scope and File Extensions | ||
|
||
A folder containing a `package.json` file, and all subfolders below that folder | ||
down until the next folder containing another `package.json`, is considered a | ||
_package scope_. The `"type"` field defines how `.js` files should be treated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need this first sentence, that defines what a package scope is. I would perhaps keep this paragraph unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the whole section on package scope and file extension belong into the package.json
section? It doesn't seem to be specific to ESM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added links to the definition of package scope in package.json.html, do you think that is sufficient?
Does the whole section on package scope and file extension belong into the package.json section? It doesn't seem to be specific to ESM.
It is actually a sub-section of Enabling, and because CJS is default in node, it makes sense to have it documented here IMO.
doc/api/package.json.md
Outdated
_package scope_. This document aims to describe the fields used by Node.js | ||
runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_package scope_. This document aims to describe the fields used by Node.js | |
runtime. | |
_package scope_. This document aims to describe the fields used by the | |
Node.js runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two sentences in this paragraph don't really belong together. The first defines a package scope, the second introduces this document.
The overall order of sections is a bit off, IMO. I would think that "name"
should come first, and there should be an explanation of how this field is used with require
and import
. When one does require('foo')
, does Node look for a folder named node_modules/foo
or a package with "name": "foo"
? What happens when they disagree? Etc. If Node doesn't use the field at all, we shouldn't be documenting the field.
After that I think should be introduction of package scope and "type"
. Then exports, conditional exports and finally "main"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall order of sections is a bit off, IMO.
I have chosen the alphabetical order to order the sections, but I reckon that's not the order most users (and even tools) do write package.json
files. I can work on that, thank you for your suggestions.
there should be an explanation of how this field (Ed:
"name"
) is used withrequire
andimport
.
AFAIK Node uses this field only for self-referencing packages, as explained here: https://github.com/nodejs/node/pull/32970/files#diff-66d7676e29759d09461d2fac50fb2564R249-R250
In order to limit the risk of confusion, maybe could we add a link to the Resolution Algorithm – or move the doc section to this document, as it doesn't belong to ESM per se. What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps in the introduction you can list all the fields that Node recognizes, in alphabetical order, with each field linked to its section? That way we can keep a section name like Conditional Exports, but in the introduction you can have something like “"exports"
: See Exports and Conditional Exports”. And then the sections can be in logical order of what makes the most sense for someone reading this document from top to bottom, but there’s still this table of contents to jump to a particular field.
As for the resolution algorithm, I think it differs in CommonJS versus ESM, so it's probably best kept in each of those sections and linked to from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps in the introduction you can list all the fields that Node recognizes, in alphabetical order, with each field linked to its section?
Wouldn't that be redundant with the TOC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, though there’s a disparity in the section names like Conditional Exports and the fields like "exports"
. We probably want to provide a way for people to look them up via either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeoffreyBooth I've made some changes, now the TOC look like this:
* `package.json`
* Introduction
* Supported fields
* `"name"`
* `"exports"`
* Subpath Exports
* Package Exports Fallbacks
* Exports Sugar
* Conditional Exports
* Nested conditions
* `"main"`
* `"type"`
Does that work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that looks great, except that I think you need "type"
before "exports"
, because doesn't "exports"
discuss topics relevant to that? The docs should make sense if someone were to read them from the top down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something, but I don't see how "type"
and "exports"
are related... "type"
is atm related to the "main"
field, but #33086 should change that.
The way I see it, its sole purpose is to set the correct parser for .js
files, which is different from other fields that affect the resolution algorithm, which is why I put it last.
@nodejs/documentation |
78beaf4
to
dd59641
Compare
dd59641
to
38d15dd
Compare
I've rebased to integrate the changes made in #33074 and #33220. Thanks to everyone who has reviewed this PR and given feedback. Is there more I can do to have it ready to land? @GeoffreyBooth, would you say I have done a good enough job of addressing your comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest edition LGTM thanks for the improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a nice refactoring to me, slowly coming around to it :) Thanks for your work and consistency pushing this forward @aduh95.
doc/api/package.json.md
Outdated
|
||
This field is ignored when the [`"exports"`][] field is provided. The `"main"` | ||
field is supported in all versions of Node.js, but its capabilities are limited: | ||
it only defines the main entry point of the package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be worth noting here that main
supports extension searching and does not require a leading ./
prefix, unlike "exports".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL you can feed an absolute path to "main"
and Node.js doesn't complain and load the script even if it's in a completely unrelated directory 🤯
Also worth noting that "main"
takes a path and "exports"
file URLs. I wasn't sure how to phrase it, so I just added Its value is interpreted as a path.
. I'm open to better suggestions x)
doc/api/package.json.md
Outdated
precedence in versions of Node.js that support `"exports"`. | ||
[Conditional Exports][] can also be used within `"exports"` to define different | ||
package entry points per environment, including whether the package is | ||
referenced via `require` or via `import`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be worth mentioning here that exports targets always start with ./
and that they must include the file extension.
Also, unlike the main, it only applies for package imports import 'pkg'
and not when loading the package path as a relative or absolute specifier - import '../pkg'
or import '/path/to/pkg.js
.
Please don't quote the above verbatim but especially given recent usability reports and having this next to the main here, noting these differences is important to highlight.
Again, this is additive to the existing docs, so can be a separate PR too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I have added a line to explicitly state that "exports"
expects file URL starting with ./
and "main"
expects a path. I also added that "exports"
takes precedence over "main"
only when referencing a package by its name. PTAL.
doc/api/esm.md
Outdated
Every package in a project’s `node_modules` folder usually contains its own | ||
[`package.json`][] file, so each project’s dependencies have their own [package | ||
scopes][]. A [`package.json`][] lacking a [`"type"`][] field is treated as if it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every package in a project’s `node_modules` folder usually contains its own | |
[`package.json`][] file, so each project’s dependencies have their own [package | |
scopes][]. A [`package.json`][] lacking a [`"type"`][] field is treated as if it | |
A folder containing a [`package.json`][] file, and all subfolders below that folder | |
down until the next folder containing another [`package.json`][], is considered a | |
_package scope_. The [`"type"`][] field defines how `.js` files should be treated | |
within a particular [`package.json`][] file’s package scope. Every package in a | |
project’s `node_modules` folder usually contains its own [`package.json`][] file, | |
so each project’s dependencies have their own [package scopes][]. A | |
[`package.json`][] lacking a [`"type"`][] field is treated as if it |
It's a little weird to jump straight to discussing node_modules
subfolders without first introducing the concept of package scope. The only difference between this version and the old version is that the first two sentences have been removed, but those are pretty vital sentences that define what a package scope is.
Also do we want to linkify every mention of package.json
and "type"
etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little weird to jump straight to discussing node_modules subfolders without first introducing the concept of package scope. The only difference between this version and the old version is that the first two sentences have been removed, but those are pretty vital sentences that define what a package scope is.
Here are my thoughts on this topic. I can see 3 ways for handling it:
- Move the sentences to
package.json.md#Introduction
, and have a link inesm.md#Package Scope and File Extensions
to the former. - Keep the sentences in
esm.md#Package Scope and File Extensions
and add link(s) inpackage.json
when package scope is mentioned. - Duplicate the sentences.
The only one that makes sense to me is 1.
, and that's the one I have implemented in this PR. I have in mind that duplication is never a good thing and package scope doesn't belong to ESM. That being said, I'm open to being persuade otherwise.
Also do we want to linkify every mention of package.json and "type" etc.?
It was simply easier for me to do a search and replace, and it seemed tedious to me to decide which mention deserves a link, and which doesn't.
doc/api/package.json.md
Outdated
<!-- YAML | ||
added: | ||
- v13.1.0 | ||
- v12.16.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be confusing since "name"
is a very old field, since the 0.x days of Node; I think it's only that Node (as opposed to npm) started using it fairly recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added based on a user request (#33143). The introduction explicitly state that the documented fields are the one used by Node.js only. Do you suggest to just remove it or to make it clearer that it can be used alongside older versions of Node.js?
Rereading all of this again, I'm struck by how much content really should be shared between docs pages. For example anyone reading the ECMAScript Modules page of the docs really should get a section on Conditional Exports, as most ESM package authors will want to know about that, but this PR moves that content into the This is making me think that maybe the best organizational structure isn't three separate pages—(CommonJS) Modules, ECMAScript Modules and |
7a5b805
to
6438ba9
Compare
New changes PTAL:
On master, the
On this PR it looks like this:
|
I tend to agree with @GeoffreyBooth's points about the structure here. Just wondering, could it be worth treating the package.json page more like a very quick reference / index, that then links to the specific sections in either Separating |
I've given this some thought, and I do think we should merge For example, I assume the Source Maps API applies to both module systems, but currently it only lives in Modules
For reference, here are the current full outlines of each page: Modules
ECMAScript Modules
|
8ae28ff
to
2935f72
Compare
4a5d6f3
to
539c33d
Compare
The changes in this PR has evolved in very different direction since it's been opened, I'm closing it to open a new one, so we can start the conversation fresh (see #34970). |
Node.js has recently started to rely more on the
package.json
file, and I feel it would be helpful to have a separate document defining the fields we use (a bit like the guide on npm docs).Currently, Conditional Exports is supported by both ESM and CJS loaders, and it is documented in the
esm.md
file, which arguably is confusing.I have used
package.json.md
for the name of the file, maybe we could use something more broad likepackage_scope.md
orpackages.md
so it might be useful for other things we want to document. Please weight in if you have thoughts.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes