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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Apr 29, 2022
+32 −77
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af15be2
refactor(cli): normalize default value application
Josh-Cena Apr 21, 2022
4546645
improve help text
Josh-Cena Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 18 additions & 67 deletions packages/docusaurus/bin/docusaurus.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,8 @@ cli
'--no-minify',
'build website without minimizing JS bundles (default: false)',
)
.action(async (siteDir, {bundleAnalyzer, config, outDir, locale, minify}) => {
build(await resolveDir(siteDir), {
bundleAnalyzer,
outDir,
config,
locale,
minify,
});
.action(async (siteDir, options) => {
build(await resolveDir(siteDir), options);
});

cli
Expand All @@ -86,9 +80,9 @@ cli
'copy TypeScript theme files when possible (default: false)',
)
.option('--danger', 'enable swizzle for unsafe component of themes')
.action(async (themeName, componentName, siteDir, options) => {
swizzle(await resolveDir(siteDir), themeName, componentName, options);
});
.action(async (themeName, componentName, siteDir, options) =>
swizzle(await resolveDir(siteDir), themeName, componentName, options),
);

cli
.command('deploy [siteDir]')
Expand All @@ -109,13 +103,9 @@ cli
'--skip-build',
'skip building website before deploy it (default: false)',
)
.action(async (siteDir, {outDir, skipBuild, config}) => {
deploy(await resolveDir(siteDir), {
outDir,
config,
skipBuild,
});
});
.action(async (siteDir, options) =>
deploy(await resolveDir(siteDir), options),
);

cli
.command('start [siteDir]')
Expand All @@ -136,18 +126,8 @@ cli
'--poll [interval]',
'use polling rather than watching for reload (default: false). Can specify a poll interval in milliseconds',
)
.action(
async (siteDir, {port, host, locale, config, hotOnly, open, poll}) => {
start(await resolveDir(siteDir), {
port,
host,
locale,
config,
hotOnly,
open,
poll,
});
},
.action(async (siteDir, options) =>
start(await resolveDir(siteDir), options),
);

cli
Expand All @@ -164,65 +144,36 @@ cli
.option('-p, --port <port>', 'use specified port (default: 3000)')
.option('--build', 'build website before serving (default: false)')
.option('-h, --host <host>', 'use specified host (default: localhost)')
.action(
async (
siteDir,
{
dir = 'build',
port = 3000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@slorber slorber Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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})`)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh... you're sure? 😄

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

Line 22 in 3b32a41

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah 😅 didn't search in utils

host = 'localhost',
build: buildSite = false,
config,
},
) => {
serve(await resolveDir(siteDir), {
dir,
port,
build: buildSite,
config,
host,
});
},
.action(async (siteDir, options) =>
serve(await resolveDir(siteDir), options),
);

cli
.command('clear [siteDir]')
.description('Remove build artifacts.')
.action(async (siteDir) => {
clear(await resolveDir(siteDir));
});
.action(async (siteDir) => clear(await resolveDir(siteDir)));

cli
.command('write-translations [siteDir]')
.description('Extract required translations of your site.')
.option(
'-l, --locale <locale>',
'the locale folder to write the translations\n"--locale fr" will write translations in ./i18n/fr folder)',
'the locale folder to write the translations.\n"--locale fr" will write translations in the ./i18n/fr folder.',
)
.option(
'--override',
'by default, we only append missing translation messages to existing translation files. This option allows to override existing translation messages. Make sure to commit or backup your existing translations, as they may be overridden',
'By default, we only append missing translation messages to existing translation files. This option allows to override existing translation messages. Make sure to commit or backup your existing translations, as they may be overridden. (default: false)',
)
.option(
'--config <config>',
'path to Docusaurus config file (default:`[siteDir]/docusaurus.config.js`)',
)
.option(
'--messagePrefix <messagePrefix>',
'allows to init new written messages with a given prefix. This might help you to highlight untranslated message to make them stand out in the UI',
'Allows to init new written messages with a given prefix. This might help you to highlight untranslated message by making them stand out in the UI (default: "")',
)
.action(
async (
siteDir,
{locale = undefined, override = false, messagePrefix = '', config},
) => {
writeTranslations(await resolveDir(siteDir), {
locale,
override,
config,
messagePrefix,
});
},
.action(async (siteDir, options) =>
writeTranslations(await resolveDir(siteDir), options),
);

cli
Expand Down
2 changes: 1 addition & 1 deletion packages/docusaurus/src/commands/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import type {HelmetServerState} from 'react-helmet-async';

export async function build(
siteDir: string,
cliOptions: Partial<BuildCLIOptions> = {},
cliOptions: Partial<BuildCLIOptions>,
// When running build, we force terminate the process to prevent async
// operations from never returning. However, if run as part of docusaurus
// deploy, we have to let deploy finish.
Expand Down
4 changes: 2 additions & 2 deletions packages/docusaurus/src/commands/clear.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ async function removePath(entry: {path: string; description: string}) {
}
}

export async function clear(siteDir: string): Promise<unknown> {
export async function clear(siteDir: string): Promise<void> {
const generatedFolder = {
path: path.join(siteDir, GENERATED_FILES_DIR_NAME),
description: 'generated folder',
Expand All @@ -40,7 +40,7 @@ export async function clear(siteDir: string): Promise<unknown> {
path: path.join(siteDir, p, '.cache'),
description: 'Webpack persistent cache folder',
}));
return Promise.all(
await Promise.all(
[generatedFolder, buildFolder, ...cacheFolders].map(removePath),
);
}
2 changes: 1 addition & 1 deletion packages/docusaurus/src/commands/deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ function shellExecLog(cmd: string) {

export async function deploy(
siteDir: string,
cliOptions: Partial<BuildCLIOptions> = {},
cliOptions: Partial<BuildCLIOptions>,
): Promise<void> {
const {outDir, siteConfig, siteConfigPath} = await loadContext({
siteDir,
Expand Down
8 changes: 5 additions & 3 deletions packages/docusaurus/src/commands/serve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ import path from 'path';
import {loadSiteConfig} from '../server/config';
import {build} from './build';
import {getCLIOptionHost, getCLIOptionPort} from './commandUtils';
import {DEFAULT_BUILD_DIR_NAME} from '@docusaurus/utils';
import type {ServeCLIOptions} from '@docusaurus/types';

export async function serve(
siteDir: string,
cliOptions: ServeCLIOptions,
cliOptions: Partial<ServeCLIOptions>,
): Promise<void> {
let dir = path.resolve(siteDir, cliOptions.dir);
const buildDir = cliOptions.dir ?? DEFAULT_BUILD_DIR_NAME;
let dir = path.resolve(siteDir, buildDir);

if (cliOptions.build) {
dir = await build(
Expand Down Expand Up @@ -69,7 +71,7 @@ export async function serve(
});
});

logger.success`Serving path=${cliOptions.dir} directory at url=${
logger.success`Serving path=${buildDir} directory at url=${
servingUrl + baseUrl
}.`;
server.listen(port);
Expand Down
4 changes: 2 additions & 2 deletions packages/docusaurus/src/commands/writeHeadingIds.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ async function getPathsToWatch(siteDir: string): Promise<string[]> {

export async function writeHeadingIds(
siteDir: string,
files?: string[],
options?: WriteHeadingIDOptions,
files: string[],
options: WriteHeadingIDOptions,
): Promise<void> {
const markdownFiles = await safeGlobby(
files ?? (await getPathsToWatch(siteDir)),
Expand Down
4 changes: 3 additions & 1 deletion packages/docusaurus/src/commands/writeTranslations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ async function writePluginTranslationFiles({

export async function writeTranslations(
siteDir: string,
options: WriteTranslationsOptions & ConfigOptions & {locale?: string},
options: Partial<
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@Josh-Cena Josh-Cena Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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))
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, makes sense.

WriteTranslationsOptions & ConfigOptions & {locale?: string}
>,
): Promise<void> {
const context = await loadContext({
siteDir,
Expand Down