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: move package config docs to separate page #34748

Closed
wants to merge 6 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 12, 2020

This part of the docs aims to contain documentation regarding package configuration that covers both ESM and CJS realms.

Refs: nodejs/modules#539

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 tools Issues and PRs related to the tools directory. label Aug 12, 2020
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

@aduh95, breaking pre-existing links is not a viable path forward — see #34663 (review). Would you mind figuring out how we will be redirecting users before continuing?

Some possibilities:

  • 301 redirects
  • Links to the new section locations below headings
  • Something else

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 12, 2020

@DerekNonGeneric thanks for the input, I agree that is something important to have in mind.

breaking pre-existing links is not a viable path forward

IIRC, it's considered OK to break things for experimental features, and this PR is focused mostly on topics in the esm.md docs (still experimental at the time of writing). So your comment is about the Addenda: Package manager tips and Core modules sections of modules.md, is that correct?

Links to the new section locations below headings

Just to be clear, are you referring to having something like Section moved to [package config page](…). for those sections? That could work for me.

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Aug 12, 2020

[…] it's considered OK to break things for experimental features […]

That's not entirely true. The APIs that are marked as experimental are subject to change (breaking), but there is no justification to entirely break the documentation page links.

So your comment is about the Addenda: Package manager tips and Core modules sections of modules.md, is that correct?

These shouldn't be entirely broken links, correct.

Just to be clear, are you referring to having something like Section moved to package config page. for those sections? That could work for me.

We have done something to this effect in the past, so it may be seen as more of an acceptable approach to take here. If you're able to find prior art similar to this in the actual API ref docs, that would be even closer to what could be seen as more acceptable.

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 12, 2020

301 redirects

I've done some digging about that, turns out that wouldn't work here: the hash part of the URL (E.G.: #modules_core_modules) is not transmitted to the server, this information is only available at the client side.

An "automatic" redirection would require involving some hacky JS stuff, I doubt that's a road we want to walk.

Anyway, I have added redirection links in my last commit, PTAL.

@guybedford
Copy link
Contributor

@DerekNonGeneric the modules documentation has not yet stabilized and we reguarlarly break the links still. I do not see why this PR should be treated differently from that perspective. Agreed changing ESM doc links should start to be blocked soon though.

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 seems like a good start. I'd be happy to land this given the comments, and then iterate on improving the esm and package config docs sections further individually. For example package configuration being a more comprehensive guide to new users eg questions like "CommonJS or ES Modules?" being discussed - "use CommonJS if...", "use ES Modules if..." etc.

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

aduh95 commented Aug 13, 2020

Based on comments from @DerekNonGeneric and @guybedford, I have removed the changes in modules.md from this PR. I have also added HTML anchors and links to the new doc to maintain backward compatibility with links to the current docs. PTAL.

@DerekNonGeneric
Copy link
Contributor

@aduh95, do you have an opinion on #34748 (comment)? 👀

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

I think it's important for this page to not imply that everyone using this file is expected to become a package author or have any remote interest in configuration of a package for any purpose whatsoever.

It may be called package.json, but I guarantee there are folks who have absolutely no interest in publishing a package and are solely interested in enabling ESM for .js extensions or adding conditional exports for the convenience of the bare specifiers for use in their apps.

That said, I think we should be cautious about the wording that is used here and try to genericize the term package.json to mean project manifest or similar. I hope that doesn't seem like an unreasonable stance on the matter but feel free to debate. :)

doc/api/package_json.md Outdated Show resolved Hide resolved
doc/api/index.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@DerekNonGeneric DerekNonGeneric added doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. labels Aug 13, 2020
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

/cc @nodejs/modules-active-members as this is a pretty substantial change

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Aug 15, 2020

I would like to ping a few folks who are involved in the development of the three main package managers who obviously use this file for packagey-type things. My impression is that it would be best to keep this page as agnostic as possible, although perhaps alluding to the fact that package.json is shared by various tools would be of some value here too.

Looking to gather some opinions if we've achieved that well enough or what some next best steps would be.

/cc @ruyadorno @shellscape @arcanis

Mainly curious to know if there are things that should belong on this page that we should make clear as it would be the first time Node.js would have a page dedicated to this file. Thanks in advance!

@DerekNonGeneric
Copy link
Contributor

I think this page still needs the "experimental API" documentation warning.

Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Nice - I'd probably just add a section mentioning that this only covers the fields that Node understands, and maybe add a link to the Yarn / npm documentations for those that belong to package managers. It might help some less advanced users that would find this page through SEO.

doc/api/package_json.md Outdated Show resolved Hide resolved
@GeoffreyBooth
Copy link
Member

Elsewhere we had discussed making this page about package authoring in general, not specific to just package.json. That way the page is free to include sections that aren't necessarily package.json fields.

Could we maybe call this page Modules: Packages? And the introduction could explain what a package is, first, before explaining what package scope is.

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 18, 2020

Nice - I'd probably just add a section mentioning that this only covers the fields that Node understands, and maybe add a link to the Yarn / npm documentations for those that belong to package managers. It might help some less advanced users that would find this page through SEO.

@arcanis right, I mean to do that in a following PR that documents package.json fields that are supported by Node.js.

Could we maybe call this page Modules: Packages? And the introduction could explain what a package is, first, before explaining what package scope is.

@GeoffreyBooth That seems like a good tradeoff to me. I'm not sure what difference there is between a package scope and a package though.

@GeoffreyBooth
Copy link
Member

I'm not sure what difference there is between a package scope and a package though.

A package is what's listed in package.json "dependencies", like "lodash". Within a package, i.e. within ./node_modules/lodash, there is at least one package scope, which is the folder that contains the package.json file: ./node_modules/lodash/package.json and all subfolders until a subfolder with another package.json file is found.

So for example if there was ./node_modules/lodash/dist/package.json, then ./node_modules/lodash/dist would be a different package scope than ./node_modules/lodash. It's similar to JavaScript when you might define a function within the body of another function: the “child” function has its own scope inside the parent function's scope.

@ruyadorno
Copy link
Member

I also think it's a good idea to loop in the folks from @nodejs/package-maintenance WG

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

aduh95 commented Aug 19, 2020

Rebased on master to solve merge conflict.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 28, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor

Thanks @isaacs for volunteering here, I just wanted to ensure there is some follow-through here that's all so things don't get dropped, not trying to be difficult and certainly didn't mean to make work for you either. I'm happy to PR my own previous comments as well further so sounds like we are good to go here then.

DerekNonGeneric pushed a commit that referenced this pull request Aug 29, 2020
This part of the docs aims to contain documentation regarding package
configuration that covers both ESM and CJS realms.

* Move Enabling section
* Update Enabling section
* Remove -u flag
* Package scopes do not carry through `node_modules` folders

Refs: nodejs/modules#539

Co-authored-by: Geoffrey Booth <webmaster@geoffreybooth.com>>
Co-authored-by: Guy Bedford <guybedford@gmail.com>

PR-URL: #34748
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@github.com>
@DerekNonGeneric
Copy link
Contributor

Landed in dbc5c17

@aduh95 aduh95 deleted the doc-package-config branch August 29, 2020 09:17
@MylesBorins
Copy link
Member

This PR doesn't land cleanly on v14.x. Before we backport I think it would be a good idea for us to ensure all the follow up PRs have been landed so this change gets released in it's best possible form.

Thoughts?

@guybedford
Copy link
Contributor

Yes, I agree we should rather we wait a little on the release of this docs work until we've completed the subsequent update PRs to a point where the majority of feedback has been incorporated and we are happy with the final state. Specifically landing #34970 and any other follow ups mentioned in this PR.

MylesBorins pushed a commit that referenced this pull request Sep 24, 2020
This part of the docs aims to contain documentation regarding package
configuration that covers both ESM and CJS realms.

* Move Enabling section
* Update Enabling section
* Remove -u flag
* Package scopes do not carry through `node_modules` folders

Refs: nodejs/modules#539

Co-authored-by: Geoffrey Booth <webmaster@geoffreybooth.com>>
Co-authored-by: Guy Bedford <guybedford@gmail.com>

PR-URL: #34748
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@github.com>
@guybedford guybedford mentioned this pull request Sep 27, 2020
3 tasks
@guybedford
Copy link
Contributor

PR for the outstanding feedback here created in #35370.

@MylesBorins MylesBorins mentioned this pull request Sep 29, 2020
aduh95 added a commit to aduh95/node that referenced this pull request Oct 23, 2020
This part of the docs aims to contain documentation regarding package
configuration that covers both ESM and CJS realms.

* Move Enabling section
* Update Enabling section
* Remove -u flag
* Package scopes do not carry through `node_modules` folders

Refs: nodejs/modules#539

Co-authored-by: Geoffrey Booth <webmaster@geoffreybooth.com>>
Co-authored-by: Guy Bedford <guybedford@gmail.com>

PR-URL: nodejs#34748
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@github.com>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
This part of the docs aims to contain documentation regarding package
configuration that covers both ESM and CJS realms.

* Move Enabling section
* Update Enabling section
* Remove -u flag
* Package scopes do not carry through `node_modules` folders

Refs: nodejs/modules#539

Co-authored-by: Geoffrey Booth <webmaster@geoffreybooth.com>>
Co-authored-by: Guy Bedford <guybedford@gmail.com>

Backport-PR-URL: #35757
PR-URL: #34748
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@github.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
This part of the docs aims to contain documentation regarding package
configuration that covers both ESM and CJS realms.

* Move Enabling section
* Update Enabling section
* Remove -u flag
* Package scopes do not carry through `node_modules` folders

Refs: nodejs/modules#539

Co-authored-by: Geoffrey Booth <webmaster@geoffreybooth.com>>
Co-authored-by: Guy Bedford <guybedford@gmail.com>

Backport-PR-URL: #35757
PR-URL: #34748
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@github.com>
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. esm Issues and PRs related to the ECMAScript Modules implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet