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

fix(create-docusaurus): give a clearer message when installation failed #6095

Merged
merged 3 commits into from
Dec 15, 2021
Merged

fix(create-docusaurus): give a clearer message when installation failed #6095

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3dada7
fix(create-docusaurus): give a clearer message when installation failed
Josh-Cena Dec 11, 2021
ded4c3d
Fix indentation
Josh-Cena Dec 11, 2021
2ddfe2a
refactor
Josh-Cena Dec 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 15 additions & 10 deletions packages/create-docusaurus/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,34 +269,32 @@ ${chalk.cyan('Creating new Docusaurus project...')}
}

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

try {
// Use force coloring the output, since the command is invoked by shelljs, which is not the interactive shell
if (
shell.exec(
`cd "${name}" && ${useYarn ? 'yarn' : 'npm install --color always'}`,
{
env: {
...process.env,
// Force coloring the output, since the command is invoked by shelljs, which is not the interactive shell
...(supportsColor.stdout ? {FORCE_COLOR: '1'} : {}),
},
},
);
} catch (err) {
console.log(chalk.red('Installation failed.'));
throw err;
).code !== 0
) {
isInstallSuccessful = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

was wondering if we couldn't abort early here? throw or `exit(1)``

That could make the workflow simpler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't want it to exit with 1. Installation is an optional step that the user can easily recover from. As long as we have a clear message, I think it's fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

then what about exit 0 :D as long as you bail out early the code becomes simpler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I see what you mean😄Yeah, makes sense

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, :)

}
}
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(`
if (isInstallSuccessful) {
console.log(`
Successfully created "${chalk.cyan(cdpath)}".
Inside that directory, you can run several commands:

Expand All @@ -319,4 +317,11 @@ We recommend that you begin by typing:

Happy building awesome websites!
`);
} else {
console.error(chalk.red('Dependency installation failed.'));
console.log(`The site directory has been created, and you can retry by typing:

${chalk.cyan('cd')} ${cdpath}
${chalk.cyan(`${pkgManager} install`)}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a good idea to print just yarn if pkgManager===yarn because yarn install doesn't make much sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yarn install makes sense. In fact I always prefer yarn install to yarn.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh! That's good then. I just read yarn instead of yarn install elsewhere, that's why I commented to ask. Awesome and sorry for nitpicking; was just looking at the PR code haha :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, thanks for the review still!

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, Josh; if there are similar beginner friendly issues; always feel free to assign and tag me! I would love to contribute too ;)

Copy link
Collaborator Author

@Josh-Cena Josh-Cena Dec 15, 2021 •
edited

Choose a reason for hiding this comment

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

We are focused on shipping the rc release so the remaining few tasks in our todo list are getting increasingly harder :/ I'll continue to delegate easier issues to the community as soon as I can think of one, so just keep updated. e.g. #6058 would be interesting to work on, as long as you have solid React skills

}
}