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: document support for package.json fields #34970

Closed
wants to merge 0 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 29, 2020

Adds package.json supported fields documentation to packages.md.

Fixes: #33143

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 29, 2020
@aduh95 aduh95 mentioned this pull request Aug 29, 2020
3 tasks
@aduh95 aduh95 force-pushed the add-package.json-doc branch 3 times, most recently from 1cbd8dc to 866748b Compare August 29, 2020 10:57
doc/api/errors.md Outdated Show resolved Hide resolved
@aduh95 aduh95 force-pushed the add-package.json-doc branch from 866748b to 46af08f Compare August 29, 2020 16:09
@guybedford
Copy link
Contributor

The big question here seems to be whether this is a reference or a guide.

I get the feeling we should treat this page as the full reference documentation, while treating the ESM modules page as the "read through guide".

As such, we should probably aim to include the full semantics of exports as full reference documentation here, including eg conditional exports.

Then rather focus on making the entry points section in the ESM documentation the clear and simple guide version for new users.

That is - I think we should focus on making this the technical content, and perhaps less on trying to make it accessible to beginners, and put the beginner focus into the ESM modules documentation page.

Let me know if that sounds like a sensible approach to you further.

This PR does touch on both which is tricky as it is two jobs in one, unfortunately the PR model doesn't fit that well for docs development! Happy to discuss ways to make it easier too.

@guybedford
Copy link
Contributor

Just reading through what we have for the ESM docs now, and of course all the guide info is in the packages page here so the ESM docs actually seem much more like a reference now to me.

Of course all the pages are both reference and guide information, but in the past the ESM page always had a flow at the top that you could follow to learn the basics all the way through (or at least that was the attempt).

If this package page is going to take that place, then we should move the "package reference" to the bottom of the docs page here rather, and treat the top of this page as more like a guide to getting started.

The other option is to make this pure reference, and structure everything by the package.json fields, as pure reference material, do the same for the ESM page, then work to create a separate modules guide.

The hard part is splitting up the material that was written the other way around so it becomes quite complex.

I don't want to make unnecessary work for anyone though, so won't obstruct progress on what people feel is best.

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@aduh95 aduh95 force-pushed the add-package.json-doc branch from 46af08f to 2e3865f Compare September 2, 2020 20:38
@aduh95
Copy link
Contributor Author

aduh95 commented Sep 2, 2020

I've moved the package.json section to the end of the document, PTAL.

As such, we should probably aim to include the full semantics of exports as full reference documentation here, including eg conditional exports.

@guybedford the documentation of conditional exports is already on the same page, do you mean merge it with the "exports" section?

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

This reordering makes all the difference, thanks @aduh95.

In terms of splitting more of the definitions as references under the package.json fields section, that is something we can do gradually in follow-up PRs to make the guide at the top shorter and as appropriate and makes sense and perhaps we don't even do that though as well.

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@aduh95 aduh95 force-pushed the add-package.json-doc branch 2 times, most recently from 63b107f to dd17bc9 Compare September 3, 2020 08:15
Copy link
Contributor

@guybedford guybedford 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 looking almost good to go to me, it would be nice to get these docs changes in before the next release, so +1 to merging sooner rather than later to keep iterating.

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.

Can you please point out where you've rewritten sections rather than simply moved them? I'd like to separate the effort of moving the sections from revising them as much as possible, so the history is easier to follow.

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor Author

aduh95 commented Sep 4, 2020

Can you please point out where you've rewritten sections rather than simply moved them?

Originally what this PR did was:

  • moving the "type"and "imports" sections
  • adding "name", "main", and "exports" sections
  • adding links to said sections in files that referenced them (errors.md, …)

Rewritten content was added to it based on suggestions from other collaborators in this thread.

I'd like to separate the effort of moving the sections from revising them as much as possible, so the history is easier to follow.

You and me both, fella ;) All suggestions were backed by a rationale that seemed reasonable to me, but I have no problem if you prefer we drop it for now.

Apologies for the aboves that slipped through (#34970 (comment)), it should be fixed now.

@GeoffreyBooth
Copy link
Member

Okay, I reread the new "exports" section and the other one, I understand now that this wasn’t a copy/paste.

This looks fine to me once we resolve #34970 (comment).

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
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.

Assuming make lint passes (I see warnings in GitHub) then this looks good to land.

@aduh95 aduh95 force-pushed the add-package.json-doc branch from 019b618 to e1470cf Compare September 7, 2020 20:18
@aduh95
Copy link
Contributor Author

aduh95 commented Sep 7, 2020

I've fixed the linter issue, and squashed the commits so the commit queue can deal with the PR 👍

@GeoffreyBooth GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 8, 2020
@nodejs-github-bot
Copy link
Collaborator

GeoffreyBooth pushed a commit that referenced this pull request Sep 12, 2020
Fixes: #33143

PR-URL: #34970
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@GeoffreyBooth
Copy link
Member

Landed in 541d296

@aduh95 aduh95 deleted the add-package.json-doc branch September 12, 2020 08:32
@ruyadorno
Copy link
Member

this changeset is currently pending the feedback items proposed on #34748 thus not landing on v14.x just yet

MylesBorins pushed a commit that referenced this pull request Sep 24, 2020
Fixes: #33143

PR-URL: #34970
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@guybedford guybedford mentioned this pull request Sep 27, 2020
3 tasks
@MylesBorins MylesBorins mentioned this pull request Sep 29, 2020
aduh95 added a commit to aduh95/node that referenced this pull request Oct 23, 2020
Fixes: nodejs#33143

PR-URL: nodejs#34970
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
Fixes: #33143

Backport-PR-URL: #35757
PR-URL: #34970
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
Fixes: #33143

Backport-PR-URL: #35757
PR-URL: #34970
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Fixes: nodejs#33143

PR-URL: nodejs#34970
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crarify which versions support package.json:"type":"module"
8 participants