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

Jump to bottom

refactor(docusaurus-init): cleanup #4571

Closed
wants to merge 2 commits into from
Closed

refactor(docusaurus-init): cleanup #4571

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
df9f4c3
chore: switch chalk -> colorette
Nytelife26 Apr 4, 2021
ebe0a70
refactor(docusaurus-init): reformat and cleanup templates
Nytelife26 Apr 4, 2021
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
16 changes: 7 additions & 9 deletions packages/docusaurus-init/bin/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,25 @@
* LICENSE file in the root directory of this source tree.
*/

const chalk = require('chalk');
const color = require('colorette');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we want colorette instead of chalk? how can you be sure that this change has no impact for all users and all OS? Why do we want this change only in one package instead of all of them?

Copy link
Author

Choose a reason for hiding this comment

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

Colorette is faster and, well, it's a better package. It doesn't make use of weird prototyping, is solely functional, and if anything provides more support with NO_COLOR (I'm not sure if Chalk does, but they don't mention it).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Colorette is faster and, well, it's a better package.

You'll have to justify your args with benchmarks and external links to back up your claims, and also explain how changing from chalk to colorette is a seamless change that is not risky. If you don't do this, I'll have to do this myself, so you put the work on me. The hard work is not to change an import, it is to assess the risk of changing that import. Everyone can change a package import and then leave when things are on fire.

If something breaks for some reason after this refactor, will you be there to fix the issues reported?
Will we live with using both colorette + chalk in the monorepo for a while or do you plan to migrate everything to colorette soon? Is there a risk that we end up with 2 libs for many years if you don't send another PR? I'd prefer to make the change atomic to the whole codebase at once if we agree that we should use colorette.

const semver = require('semver');
const path = require('path');
const program = require('commander');
const {default: init} = require('../lib');
const requiredVersion = require('../package.json').engines.node;

if (!semver.satisfies(process.version, requiredVersion)) {
console.log(
chalk.red(`\nMinimum node version not met :)`) +
chalk.yellow(
`\nYou are using Node ${process.version}, Requirement: Node ${requiredVersion}.\n`,
),
);
console.log(`${color.red('Minimum Node version not met :)')}
${color.yellow(
`You are using Node ${process.version} - requirement: ${requiredVersion}.`,
)}`);
process.exit(1);
}

function wrapCommand(fn) {
return (...args) =>
fn(...args).catch((err) => {
console.error(chalk.red(err.stack));
console.error(color.red(err.stack));
process.exitCode = 1;
});
}
Expand All @@ -50,7 +48,7 @@ program

program.arguments('<command>').action((cmd) => {
program.outputHelp();
console.log(` ${chalk.red(`\n Unknown command ${chalk.yellow(cmd)}.`)}`);
console.log(` ${color.red(`\n Unknown command ${color.yellow(cmd)}.`)}`);
console.log();
});

Expand Down
2 changes: 1 addition & 1 deletion packages/docusaurus-init/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
},
"license": "MIT",
"dependencies": {
"chalk": "^4.1.0",
"colorette": "^1.2.2",
"commander": "^5.1.0",
"fs-extra": "^9.1.0",
"lodash": "^4.17.20",
Expand Down
105 changes: 48 additions & 57 deletions packages/docusaurus-init/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
* LICENSE file in the root directory of this source tree.
*/

import chalk from 'chalk';
import colorette from 'colorette';
import fs from 'fs-extra';
import {execSync} from 'child_process';
import prompts, {Choice} from 'prompts';
import path from 'path';
import shell from 'shelljs';
import {kebabCase} from 'lodash';
import type {CliOptions} from './types/index';

function hasYarn(): boolean {
try {
Expand All @@ -22,8 +23,8 @@ function hasYarn(): boolean {
}
}

function isValidGitRepoUrl(gitRepoUrl: string): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there is a significant perf issue in docusaurus-init. Do we really need to refactor this file in subtle ways (like changing Regexes), particularly when this part of the codebase has not a strong code coverage?

Copy link
Author

Choose a reason for hiding this comment

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

It could still perform better and if it can, why not go for it? Plus, the code does the same thing, there is no need for additional testing as it is synonymous but a cleaner and more performant version. Code quality should be a priority.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not against these changes, but then please add unit tests.

Plus, the code does the same thing

That's what it looks like (unless there is a hidden typo, which is always a possibility with Regexes) but any change without a test ends up being a risky change for me.

return ['https://', 'git@'].some((item) => gitRepoUrl.startsWith(item));
function isValidGitRepoUrl(gitRepoUrl?: string): boolean {
return /^((https|git):\/\/)|(git@)/g.test(gitRepoUrl ?? '');
}

async function updatePkg(
Expand All @@ -41,27 +42,22 @@ export default async function init(
rootDir: string,
siteName?: string,
reqTemplate?: string,
cliOptions: Partial<{
useNpm: boolean;
skipInstall: boolean;
}> = {},
cliOptions: CliOptions = {},
): Promise<void> {
const useYarn = !cliOptions.useNpm ? hasYarn() : false;
const useYarn = !cliOptions.useNpm && hasYarn();
const templatesDir = path.resolve(__dirname, '../templates');
const templates = fs
.readdirSync(templatesDir)
.filter((d) => !d.startsWith('.') && !d.startsWith('README'));
const templates = [];
// eslint-disable-next-line no-restricted-syntax
for await (const entry of await fs.opendir(templatesDir)) {
if (entry.isDirectory()) {
templates.push(entry.name);
}
}

function makeNameAndValueChoice(value: string): Choice {
function makeChoice(value: string): Choice {
return {title: value, value} as Choice;
}

const gitChoice = makeNameAndValueChoice('Git repository');
const templateChoices = [
...templates.map((template) => makeNameAndValueChoice(template)),
gitChoice,
];

let name = siteName;

// Prompt if siteName is not passed from CLI.
Expand All @@ -76,12 +72,12 @@ export default async function init(
}

if (!name) {
throw new Error(chalk.red('A site name is required'));
throw new Error(colorette.red('A site name is required'));
}

const dest = path.resolve(rootDir, name);
if (fs.existsSync(dest)) {
throw new Error(`Directory already exists at ${dest} !`);
throw new Error(`Directory already exists at ${dest}!`);
}

let template = reqTemplate;
Expand All @@ -91,9 +87,12 @@ export default async function init(
type: 'select',
name: 'template',
message: 'Select a template below...',
choices: templateChoices,
choices: [
...templates.map((templateDir) => makeChoice(templateDir)),
makeChoice('Git repository'),
],
});
template = templatePrompt.template;
template = templatePrompt.template as string;
}

// If user choose Git repository, we'll prompt for the url.
Expand All @@ -102,36 +101,36 @@ export default async function init(
type: 'text',
name: 'gitRepoUrl',
validate: (url?: string) => {
if (url && isValidGitRepoUrl(url)) {
if (isValidGitRepoUrl(url)) {
return true;
}
return chalk.red(`Invalid repository URL`);
return colorette.red(`Invalid repository URL`);
},
message:
'Enter a repository URL from GitHub, BitBucket, GitLab, or any other public repo. \n(e.g: https://github.com/ownerName/repoName.git)',
'Enter a repository URL from GitHub, BitBucket, GitLab, or any other public repo.\n(e.g: https://github.com/owner/repo.git)',
});
template = repoPrompt.gitRepoUrl;
template = repoPrompt.gitRepoUrl as string;
}

console.log();
console.log(chalk.cyan('Creating new Docusaurus project ...'));
console.log();
console.log(`\n${colorette.cyan('Creating new Docusaurus project ...')}\n`);

if (template && isValidGitRepoUrl(template)) {
console.log(`Cloning Git template: ${chalk.cyan(template)}`);
if (isValidGitRepoUrl(template)) {
console.log(`Cloning Git template: ${colorette.cyan(template)}`);
if (
shell.exec(`git clone --recursive ${template} ${dest}`, {silent: true})
.code !== 0
) {
throw new Error(chalk.red(`Cloning Git template: ${template} failed!`));
throw new Error(
colorette.red(`Cloning Git template: ${template} failed!`),
);
}
} else if (template && templates.includes(template)) {
} else if (templates?.includes(template)) {
// Docusaurus templates.
try {
await fs.copy(path.resolve(templatesDir, template), dest);
} catch (err) {
console.log(
`Copying Docusaurus template: ${chalk.cyan(template)} failed!`,
`Copying Docusaurus template: ${colorette.cyan(template)} failed!`,
);
throw err;
}
Expand All @@ -147,7 +146,7 @@ export default async function init(
private: true,
});
} catch (err) {
console.log(chalk.red('Failed to update package.json'));
console.log(colorette.red('Failed to update package.json'));
throw err;
}

Expand All @@ -164,41 +163,33 @@ export default async function init(

const pkgManager = useYarn ? 'yarn' : 'npm';
if (!cliOptions.skipInstall) {
console.log(`Installing dependencies with: ${chalk.cyan(pkgManager)}`);
console.log(`Installing dependencies with: ${colorette.cyan(pkgManager)}`);

try {
shell.exec(`cd "${name}" && ${useYarn ? 'yarn' : 'npm install'}`);
} catch (err) {
console.log(chalk.red('Installation failed'));
console.log(colorette.red('Installation failed'));
throw err;
}
}
console.log();

// Display the most elegant way to cd.
const cdpath =
path.join(process.cwd(), name) === dest
? name
: path.relative(process.cwd(), name);

console.log();
console.log(`Success! Created ${chalk.cyan(cdpath)}`);
console.log('Inside that directory, you can run several commands:');
console.log();
console.log(chalk.cyan(` ${pkgManager} start`));
console.log(' Starts the development server.');
console.log();
console.log(chalk.cyan(` ${pkgManager} ${useYarn ? '' : 'run '}build`));
console.log(`\n\nSuccess! Created ${colorette.cyan(cdpath)}`);
console.log('Inside that directory, you can run several commands:\n');
console.log(` ${colorette.cyan(`${pkgManager} start`)}`);
console.log(' Starts the development server.\n');
console.log(
` ${colorette.cyan(`${pkgManager} ${!useYarn && 'run '}build`)}`,
);
console.log(' Bundles the app into static files for production.');
console.log();
console.log(chalk.cyan(` ${pkgManager} deploy`));
console.log(' Publish website to GitHub pages.');
console.log();
console.log('We suggest that you begin by typing:');
console.log();
console.log(chalk.cyan(' cd'), cdpath);
console.log(` ${chalk.cyan(`${pkgManager} start`)}`);

console.log();
console.log(` ${colorette.cyan(`${pkgManager} deploy`)}`);
console.log(' Publish website to GitHub pages.\n');
console.log('We suggest that you begin by typing:\n');
console.log(` ${colorette.cyan('cd')} ${cdpath}`);
console.log(` ${colorette.cyan(`${pkgManager} start`)}\n`);
console.log('Happy hacking!');
}
11 changes: 11 additions & 0 deletions packages/docusaurus-init/src/types/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

export interface CliOptions {
useNpm?: boolean;
skipInstall?: boolean;
}
8 changes: 8 additions & 0 deletions packages/docusaurus-init/src/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

export * from './cli';
6 changes: 3 additions & 3 deletions packages/docusaurus-init/templates/README.MD → packages/docusaurus-init/templates/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Templates

Official templates provided by Docusaurus. They are designed to be selected when using the `npx @docusaurus/init init [name] [template]` CLI command.
Official templates provided by Docusaurus. They are designed to be selected when using the `npx @docusaurus/init init [name] [template]` command.

## Guide to Test Templates for Developer
## How do I test templates?

1. `yarn install` in the root of the repo (one level above this directory).
1. Run `yarn install` in the root of the repo.
1. Go to any template's directory, example: `cd classic && yarn start`.
16 changes: 8 additions & 8 deletions packages/docusaurus-init/templates/bootstrap/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,30 +4,30 @@ This website is built using [Docusaurus 2](https://docusaurus.io/), a modern sta

### Installation

```
$ yarn
```sh
yarn install
```

### Local Development

```
$ yarn start
```sh
yarn start
```

This command starts a local development server and open up a browser window. Most changes are reflected live without having to restart the server.

### Build

```
$ yarn build
```sh
yarn build
```

This command generates static content into the `build` directory and can be served using any static contents hosting service.

### Deployment

```
$ GIT_USER=<Your GitHub username> USE_SSH=true yarn deploy
```sh
GIT_USER=<Your GitHub username> USE_SSH=true yarn deploy
```

If you are using GitHub pages for hosting, this command is a convenient way to build the website and push to the `gh-pages` branch.
3 changes: 2 additions & 1 deletion packages/docusaurus-init/templates/bootstrap/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
"@mdx-js/react": "^1.5.8",
"classnames": "^2.2.6",
"react": "^17.0.1",
"react-dom": "^17.0.1"
"react-dom": "^17.0.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

theme bootstrap is unmaintained currently, we'll make it production ready later but it's not worth to update it atm

"react-if": "^4.0.1"
},
"browserslist": {
"production": [
Expand Down
9 changes: 5 additions & 4 deletions packages/docusaurus-init/templates/bootstrap/src/pages/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import Layout from '@theme/Layout';
import Link from '@docusaurus/Link';
import useDocusaurusContext from '@docusaurus/useDocusaurusContext';
import useBaseUrl from '@docusaurus/useBaseUrl';
import {If} from 'react-if';

import styles from './styles.module.css';

Expand Down Expand Up @@ -44,11 +45,11 @@ function Feature({imageUrl, title, description}) {
const imgUrl = useBaseUrl(imageUrl);
return (
<div className={classnames('col col--4', styles.feature)}>
{imgUrl && (
<If condition={imgUrl}>
<div className="text--center">
<img className={styles.featureImage} src={imgUrl} alt={title} />
</div>
)}
</If>
<h3>{title}</h3>
<p>{description}</p>
</div>
Expand All @@ -72,15 +73,15 @@ export default function Home() {
</div>
</header>
<main>
{features && features.length > 0 && (
<If condition={features?.length}>
<section className={styles.section}>
<div className={styles.features}>
{features.map((props, idx) => (
<Feature key={idx} {...props} />
))}
</div>
</section>
)}
</If>
</main>
</div>
</Layout>
Expand Down
Loading