refactor(cli): normalize the application of default option values

[Josh-Cena]

Motivation

Right now, our CLI code is a bit messy, because default values are applied at different stages. Sometimes we apply default values directly in the command action; sometimes we pretend an argument of a function is optional while in practice it always exists... I decided to decouple the CLI implementation from the actual command interface, i.e. the CLI only passes the arguments down, without caring what's inside it. This should make the code much cleaner.

A big pain point of commander, though, is that its typings are quite loose. I can't find a great way to work around it at the moment.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

All commands work the same as before.

[netlify]

✅ [V2]

Name	Link
🔨 Latest commit	4546645
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/626b6d7b6d914b000b930c81
😎 Deploy Preview	https://deploy-preview-7220--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	56
🟢 Accessibility	100
🟠 Best practices	83
🟢 SEO	100
🟢 PWA	90

Lighthouse ran on https://deploy-preview-7220--docusaurus-2.netlify.app/

[github-actions]

Size Change: 0 B

Total Size: 803 kB

ℹ️ View Unchanged

Filename	Size
website/.docusaurus/globalData.json	50.1 kB
website/build/assets/css/styles.********.css	107 kB
website/build/assets/js/main.********.js	607 kB
website/build/index.html	38.8 kB

_{compressed-size-action}

[slorber]

not fan of this kind of change 🤷‍♂️ I'd rather do the opposite and ensure all commands are never partial => handle edge cases and apply defaults at the edge of the system

[Josh-Cena]

The problem is that writeTranslations should not be seen as fully internal. It's exported from the main interface and users may import @docusaurus/core and use it as a programmatic API. Therefore, I'd keep maximum compatibility with the CLI interface.

[Josh-Cena]

Related: #4841 We should always assume the existence of those users that don't interact through the CLI. Even if it's technically not documented usage, I don't see why it makes the architecture any different if we apply defaults within docusuarus.mjs or writeTranslations.ts—as long as we only apply it once. NOT doing it in docusuarus.mjs means (a) more permissive programmatic API and (b) a CLI registry that's easier to maintain in the long term because it's not intertwined with the implementation, but only a very thin wrapper around each exported function.

[slorber]

ok, that seems reasonable

What I don't like is to have long methods receiving partial options, where there's always a risk to end up using the partial option on which default was not properly applied

Can we split the methods in 2, something like this?

function doWriteTranslations(siteDir: string, options: Options) {
	// actual code
} 

export function writeTranslations(siteDir: string, options: Partial<Options>) {
	return doWriteTranslations(siteDir, applyDefaults(options))
} 

[Josh-Cena]

Yes, makes sense.

[slorber]

where are these default values being applied now?

isn't it convenient that the default value and the doc message mentioning it are co-located?

Why not doing the opposite instead: always apply all default values in this file, ensuring we don't have to handle partial options anywhere else in the system?

[Josh-Cena]

These have always been applied internally, before and after. These default values actually only serve the purpose of being confusing & overly defensive.

[slorber]

as far as I understand this is the only place where 3000 is defined.

Otherwise it's the default of the underlying lib, which as you can see, can change over time (vercel/serve#680)

I'd rather keep applying these defaults explicitly so that we don't depend on underlying deps changes.

Also we can use the default options in commander help messages

  .option('-p, --port <port>', `use specified port (default: ${DefaultServeOptions.port})`)

[Josh-Cena]

Oh... you're sure? 😄

docusaurus/packages/docusaurus/src/commands/commandUtils.ts

Line 22 in 3b32a41

const basePort = portOption ? parseInt(portOption, 10) : DEFAULT_PORT;

[slorber]

ah 😅 didn't search in utils

[Josh-Cena]

Actually... many of the default values are applied again in the underlying utility functions, which is a good thing, because these utility functions are re-used in multiple places where the concerns about the option values are different. The functions in src/commands are just a pipe-through, so the default values probably shouldn't be applied there. In any case, removing the default values in bin/docusaurus.mjs is a must-have since it's just very confusing that way when the utility functions would apply the default values again.