-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: merge CJS and ESM docs #33512
doc: merge CJS and ESM docs #33512
Conversation
Thanks for doing this. Because the diff is huge, and GitHub doesn't make clear what text was simply moved as opposed to moved and modified, do you mind listing which sections in the new |
Splitting it into separate commits might help. We will also need some sort of redirect from |
Documents package.json supported fields. Fixes: nodejs#33143
6625765
to
a3029c7
Compare
I've split it into several commits as @devsnek suggested, each one with a small enough diff for GitHub UI.
That would be on Node.js website side, right?
Some section have changed names:
The content of the sections has not changed (except minor stuff like links and eslint comments). |
8ae28ff
to
2935f72
Compare
For this to land I think we need to ensure the following:
I still have some concerns about putting everything together in one big doc page from a maintenance and digestibility point of view, but I can certainly get behind it if others agree it is cleaner. |
Hey, going back to this PR, it seems it requires a non negligible amount of work to fix the merge conflicts and have it ready for review. I'd like to know if the general feeling among Node.js collaborators is that this idea is worth pursuing before investing my time in it. Specifically, some concerns have been raised regarding the length of the resulting document, is this going to be a deal breaker? Based on the comments from @GeoffreyBooth and @guybedford, here is the structure I'm aiming for: TOCModules
|
The more I think about this the more I think it's the wrong call to combine the CJS and ESM documentation. I think in general we should be splitting API pages up into smaller chunks not combining them into larger ones. It seems unnecessary for users writing ES modules to need to use the same documentation page as the CommonJS docs which are also much more well established and have URLs and links that shouldn't be changed. Perhaps we can take this same initiative in the other direction - and split out the shared content into its own page rather? I liked the proposal previously of a separate package.json API page that is used as the point of linkage between CJS and ESM resolution. @aduh95 do you think your outline could adapt to that sort of approach here, or does that seem too big of a shift? Sorry for not being clearer with my opinion from the start here. Hard questions! Feedback from others welcome too! |
@guybedford I tend to agree with you, especially if we take into account the maintainability of it all. What do you all think of creating a new |
We could maybe rename the pages like “Modules: CommonJS” and “Modules: ECMAScript” so that they’re next to each other in the table of contents. I discussed with @guybedford a bit and we were thinking that maybe the page for common stuff could be organized around information for package authors? So like “Packages” or “Modules: Authoring Packages” or somesuch. Then it doesn’t need to be so closely tied to This would leave source maps as the odd section out, that's used in both module systems but unrelated to authoring packages. We're thinking that the source maps info doesn't really belong on any of these pages, as it's not really related to modules or packages at all; it could maybe move to the Debugger page or even its own top-level page. |
@aduh95 thanks, I'm glad to hear you don't find the feedback too disheartening!
This makes sense. Ideally we would set up a redirect for Perhaps lets open a new issue with the proposal for a |
Also what would happen to the docs on the |
Closing this, see nodejs/modules#539 for more context. |
Based on @GeoffreyBooth's suggestion in #32970 (comment).
Documents package.json supported fields.
Fixes: #33143
Alternate to #32970
Problem this PR is trying to solve
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, there are features supported by both ESM and CJS loaders, and it is documented in one arbitrary file (E.G.: Conditional Exports is documented in
esm.md
only, Source Maps API inmodules.md
only, …).This PR merges the two documents (as well as the
package.json
documentation from #32970) in themodules.md
file.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes