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

feat(learn): add Package Configuration article #7229

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

JakobJingleheimer
Copy link
Member

This is an article I wrote a couple years ago. It's rather popular and addresses an issue @joyeecheung brought up at the collab summit.

I think canonical url needs to be set for SEO, so Google doesn't think it's plagiarised, but I don't know how to do that here.

@JakobJingleheimer JakobJingleheimer requested a review from a team as a code owner November 14, 2024 20:38
Copy link

vercel bot commented Nov 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Nov 16, 2024 1:46pm

@AugustinMauroy
Copy link
Member

I think canonical url needs to be set for SEO, so Google doesn't think it's plagiarised, but I don't know how to do that here.

OH idk if we can handle that with ou current infra

@JakobJingleheimer
Copy link
Member Author

I think canonical url needs to be set for SEO, so Google doesn't think it's plagiarised, but I don't know how to do that here.

OH idk if we can handle that with ou current infra

It's an html tag. But I dunno how to include that via markdown.

https://developers.google.com/search/docs/crawling-indexing/consolidate-duplicate-urls

@AugustinMauroy
Copy link
Member

we already have that for lang

@JakobJingleheimer
Copy link
Member Author

we already have that for lang

Soooo… should I add something like

canonical: https://…

@AugustinMauroy
Copy link
Member

Soooo… should I add something like

Sadly no because canonical for lang is handle by our i18n engine.

@AugustinMauroy
Copy link
Member

AugustinMauroy commented Nov 14, 2024

the only front matter that learn section handle is:

--- 
title: A super cool title
layout: learn
authors: github_username, another_github_username
--- 

but why not open an issue if needed

apps/site/pages/en/learn/modules/package-configuration.md Outdated Show resolved Hide resolved

A frequent question is “how do I make this work!?” (often with angry tears); but yet more frequently we come across packages that are just misconfigured.

All the provided `package.json` configurations (not specifically marked “does not work”) work in Node.js 12.22.x (v12 latest, the oldest supported line) and 17.2.0 (current latest at the time)<sup>[1](#footnotes)</sup>, and for grins, with webpack 5.53.0 and 5.63.0 respectively. These are available: [nodejs/package-examples](https://github.com/nodejs/package-examples/blob/main/config).
Copy link
Member

Choose a reason for hiding this comment

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

Can you ensure the examples work on 22 and 23, and update this sentence accordingly? (and throughout the file)

Those users are (IMO) the "target" audience.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 15, 2024

Thanks for picking it up! I won't have time to review this until ~December, so feel free to go ahead without my approval if you want to get it out soonish. But some observations after a quick glance, which I think should be addressed given that we spent hours at the summit talking about the issues with outdated content...:

  1. This doesn't reference a popular pattern that e.g. Babel uses to avoid the dual package hazard: Recommend node/default conditions instead of require/import as a solution to the dual package hazard node#52174
  2. Somewhere in the beginning of the article, it should make it clear when no configuration is necessary. Generally you don't need to use the exports field at all if you just have an CJS exports (use main) and a normal package.json.
  3. It's missing instructions of shipping ESM on Node.js versions >= 23 (and soon >= 22 or >= 20) while maintaining compatibility for CJS users via require(esm).
  4. When surveying the exports patterns in the ecosystem I have very rarely seen the package.json specified. On the other hand, types get specified quite a lot. Maybe that's something worth teaching here too.

{
"type": "commonjs", // current default, but may change
"engines": { "node": ">=12.22.7" }, // optional, but kind
"exports": {
Copy link
Member

@joyeecheung joyeecheung Nov 15, 2024

Choose a reason for hiding this comment

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

Is this necessary? At most it can use the explicit type field, but AFAICT you just need to point the main field and don't need to bother with exports.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe technically yes. But this guide is about should not necessarily must.

Copy link
Member

@joyeecheung joyeecheung Nov 16, 2024

Choose a reason for hiding this comment

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

That may be true before it's moved to the website, but now it's in the learn session I think we should target the audience differently (which I think was the core of the doc/ambassador program in the past two summits): we should teach more best practices that are approachable to beginners and up-to-date. Having the guide about package shipping very pro-configuration when it's not necessary and is not what the majority of the ecosystem does is deviating from that goal IMO.

Having to configure exports is already not great UX and it's only a compromise/advanced topic for those who needs more control over the package, but for many simpler cases, the package author should not need to pay for the mental burden of it at all. I have seen many complaints/jokes from end users and maintainers about the mental burden of the exports field, and I think we should make it clear in the website tutorials about when they don't have to pay for it to reduce barriers.

@JakobJingleheimer
Copy link
Member Author

JakobJingleheimer commented Nov 15, 2024

@joyeecheung there was talk a while ago about discouraging "main" in favour of "exports". I'm not familiar with the rationale behind it ("main" seems very okay to me without some of the drawbacks of "exports"); that's the only reason I use "exports" in the examples what aren't actually using any fanciness from exports (and "main" would be equivalent). Is that not the case? I have no preference whatsoever.

@RedYetiDev RedYetiDev added content Issues/pr concerning content learn Issues/pr concerning the learn section labels Nov 15, 2024

### CJS source and both CJS & ESM distribution

You have a few options:
Copy link
Member

Choose a reason for hiding this comment

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

this seems to presume you have multiple things to export - is there an example of when you only have one thing to export? (ie, a single-responsibility package)


# Package configuration

Configuration is always a chore, but an unfortunately necessary evil. And configuring a package for CommonJS (CJS) and ES Modules (ESM) can be a waking nightmare—not least because it has changed a dozen times in half as many years.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Configuration is always a chore, but an unfortunately necessary evil. And configuring a package for CommonJS (CJS) and ES Modules (ESM) can be a waking nightmarenot least because it has changed a dozen times in half as many years.
Configuring a node.js project/package is always a chore, but an unfortunate necessary evil. And configuring a package for CommonJS (CJS) and ES Modules (ESM) can be a waking nightmare - not least because it has changed a dozen times in half as many years.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i dont think this is better in this case

Copy link
Member

Choose a reason for hiding this comment

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

I think unfortunately -> unfortunate is better (seeing as unfortunate is an adjective, rather than an adverb).


An additional complication is bundlers, which historically managed much of this territory. However, much of what we previously needed bundle(r)s to manage is now native functionality; yet bundlers are still (and likely always will be) necessary for some things. Unfortunately, functionality bundlers no-longer need to provide is deeply ingrained in older bundlers’ implementations, so they can at times be too helpful, and in some cases, anti-pattern (bundling a library is often not recommended by bundler authors themselves). The hows and whys of that are an article unto itself.

## Pick your poison
Copy link
Member

Choose a reason for hiding this comment

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

I understand the idea behind the title, but the name Node.js doesn't really sell it. May be something like "Choosing what best suits our expectations "

```json
{
"type": "commonjs", // current default, but may change
"engines": { "node": ">=12.22.7" }, // optional, but kind
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"engines": { "node": ">=12.22.7" }, // optional, but kind
"engines": { "node": ">=12.22.7" }, // optional, it lets you know which node version you're using and npm warns you when it's not the right version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea but this comment is very long (and will cause horizontal scrolling). I think it would be better to add a general note somewhere pointing to the npm doc that explains what this field does.


The "Gin & Tonic" of packages: This takes a small bit of finesse but is also pretty straight-forward.

**Working example**: [cjs-with-esm-distro](https://github.com/JakobJingleheimer/nodejs-module-config-examples/tree/main/packages/cjs/esm-distro)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Working example**: [cjs-with-esm-distro](https://github.com/JakobJingleheimer/nodejs-module-config-examples/tree/main/packages/cjs/esm-distro)
**Working example**: [cjs-with-esm-distro](https://github.com/JakobJingleheimer/nodejs-module-config-examples/tree/main/packages/cjs/esm-distro) _unofficial example node.js_

Add this mention to clarify that the project trust the source but it's not officially maintianed by node.js itself.
yeah i know your are on node.js But I'm convicted that a good practice to explain when it's not a node.js official ressource

Copy link
Member

Choose a reason for hiding this comment

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

I think there are other link in this article can be updated with this mention

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good catch. I intend to move these into the nodejs/package-examples repo joyee started.

Copy link

github-actions bot commented Nov 16, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 99 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 100 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 99 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

Copy link

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 91%
90.38% (592/655) 75.43% (172/228) 94.26% (115/122)

Unit Test Report

Tests Skipped Failures Errors Time
132 0 💤 0 ❌ 0 🔥 5.17s ⏱️

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

Soft LGTM, but I would appreciate more reviewers first.

@ovflowd
Copy link
Member

ovflowd commented Nov 16, 2024

BTW I love your writing style, and would love if more existing content got updated with this nice writing style.


## Preamble: How did we get here

CommonJS (CJS) was created _long_ before ECMAScript Modules (ESM), back when JavaScript was still adolescent—CJS and jQuery were created just 3 years apart. CJS is not an official (TC39) standard and is supported by a limited few platforms (most notably, Node.js). ESM as a standard has been incoming for several years; it is currently supported by all major platforms (browsers, Deno, Node.js, etc), meaning it will run pretty much everywhere. As it became clear ESM would effectively succeed CJS (which is still very popular and widespread), many attempted to adopt early on, often before a particular aspect of the ESM specification was finalised. Because of this, those changed over time as better information became available (often informed by learnings/experiences of those eager beavers), going from best-guess to the aligning with the specification.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CommonJS (CJS) was created _long_ before ECMAScript Modules (ESM), back when JavaScript was still adolescent—CJS and jQuery were created just 3 years apart. CJS is not an official (TC39) standard and is supported by a limited few platforms (most notably, Node.js). ESM as a standard has been incoming for several years; it is currently supported by all major platforms (browsers, Deno, Node.js, etc), meaning it will run pretty much everywhere. As it became clear ESM would effectively succeed CJS (which is still very popular and widespread), many attempted to adopt early on, often before a particular aspect of the ESM specification was finalised. Because of this, those changed over time as better information became available (often informed by learnings/experiences of those eager beavers), going from best-guess to the aligning with the specification.
CommonJS (CJS) was created _long_ before ECMAScript Modules (ESM), back when JavaScript was still adolescent—CJS and jQuery were created just 3 years apart. CJS is not an official ([TC39](https://tc39.es)) standard and is supported by a limited few platforms (most notably, Node.js). ESM as a standard has been incoming for several years; it is currently supported by all major platforms (browsers, Deno, Node.js, etc), meaning it will run pretty much everywhere. As it became clear ESM would effectively succeed CJS (which is still very popular and widespread), many attempted to adopt early on, often before a particular aspect of the ESM specification was finalised. Because of this, those changed over time as better information became available (often informed by learnings/experiences of those eager beavers), going from best-guess to the aligning with the specification.

May be ad link to tc39


The [`.mjs`](https://nodejs.org/api/esm.html#enabling) file extension is a trump-card: it will override **any** other configuration and the file will be treated as ESM. Using this file extension is necessary because `packageJson.exports.import` does **NOT** signify that the file is ESM (contrary to common, if not universal, misperception), only that it is the file to be used when the package is imported (ESM _can_ import CJS. See [Gotchas](#gotchas) below).

The [`"engines"`](https://nodejs.dev/learn/the-package-json-guide#engines) field provides both a human-friendly and a machine-friendly indication of with which version(s) of Node.js the package is compatible. Depending on the package manager used, an exception may be thrown causing the installation to fail when the consumer is using an incompatible version of Node.js (which can be very helpful to consumers). Including this field here will save a lot of headache for consumers with an older version of Node.js who cannot use the package.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The [`"engines"`](https://nodejs.dev/learn/the-package-json-guide#engines) field provides both a human-friendly and a machine-friendly indication of with which version(s) of Node.js the package is compatible. Depending on the package manager used, an exception may be thrown causing the installation to fail when the consumer is using an incompatible version of Node.js (which can be very helpful to consumers). Including this field here will save a lot of headache for consumers with an older version of Node.js who cannot use the package.
The [`"engines"`](https://nodejs.dev/learn/the-package-json-guide#engines) field provides both a human-friendly and a machine-friendly indication of with which version(s) of Node.js the package is compatible. Depending on the package manager used, an exception may be thrown causing the installation to fail when the consumer is using an incompatible version of Node.js (which can be very helpful to consumers). Including this field here will save a lot of headache for consumers with an older version of Node.js who cannot use the package.

"https://nodejs.dev/learn/the-package-json-guide#engines" = 404
I'm looking if we still have this docs

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 3 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • apps/site/navigation.json: Language not supported
  • packages/i18n/locales/en.json: Language not supported
@RedYetiDev
Copy link
Member

Can you update the examples to use v22, which is the current LTS? I don't think it's helpful to users if this articles targets v12.

Comment on lines +401 to +402

## Footnotes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Footnotes

@RedYetiDev

This comment was marked as outdated.

@AugustinMauroy
Copy link
Member

@RedYetiDev the orignal post was written by jakob who put it on the learn section. And Jakob propose that at the collab summit. So I don't think it's an problem here

@JakobJingleheimer
Copy link
Member Author

Yeah, I'm the original author, so posting external content is not an issue here: it's becoming internal content.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 18, 2024

there was talk a while ago about discouraging "main" in favour of "exports".

AFAIK the primary concern about "main" is that it does not forbid files to be loaded by dependencies, whereas "exports" can be used to guard off unwanted loads. But then that doesn't apply to all packages, especially the small ones that have no files that are internal to begin with. It would be pretty daunting for the package configuration guide to just start out with "let's do exports!" given that so many people complain about the mental burden of it. A tutorial should start small and simple and only reach to advanced practices as it goes IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Issues/pr concerning content learn Issues/pr concerning the learn section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants