-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Conversation
Review requested:
|
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.
@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
@DerekNonGeneric thanks for the input, I agree that is something important to have in mind.
IIRC, it's considered OK to break things for experimental features, and this PR is focused mostly on topics in the
Just to be clear, are you referring to having something like |
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.
These shouldn't be entirely broken links, correct.
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. |
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. |
@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. |
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 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.
Based on comments from @DerekNonGeneric and @guybedford, I have removed the changes in |
@aduh95, do you have an opinion on #34748 (comment)? 👀 |
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 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. :)
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.
/cc @nodejs/modules-active-members as this is a pretty substantial change
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 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! |
I think this page still needs the "experimental API" documentation warning. |
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.
Elsewhere we had discussed making this page about package authoring in general, not specific to just Could we maybe call this page |
@arcanis right, I mean to do that in a following PR that documents
@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. |
A package is what's listed in So for example if there was |
I also think it's a good idea to loop in the folks from @nodejs/package-maintenance WG |
1bc95aa
to
ed01e9d
Compare
Rebased on master to solve merge conflict. |
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. |
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 <[email protected]>> Co-authored-by: Guy Bedford <[email protected]> PR-URL: #34748 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruy Adorno <[email protected]>
Landed in dbc5c17 |
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? |
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. |
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 <[email protected]>> Co-authored-by: Guy Bedford <[email protected]> PR-URL: #34748 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruy Adorno <[email protected]>
PR for the outstanding feedback here created in #35370. |
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 <[email protected]>> Co-authored-by: Guy Bedford <[email protected]> PR-URL: nodejs#34748 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruy Adorno <[email protected]>
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 <[email protected]>> Co-authored-by: Guy Bedford <[email protected]> Backport-PR-URL: #35757 PR-URL: #34748 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruy Adorno <[email protected]>
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 <[email protected]>> Co-authored-by: Guy Bedford <[email protected]> Backport-PR-URL: #35757 PR-URL: #34748 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruy Adorno <[email protected]>
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), orvcbuild test
(Windows) passes