-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
refactor(v2): add exception handling if external command is fails #4870
Conversation
✔️ [V2] 🔨 Explore the source changes: 0953226 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60b7d5283b293e0007eec1f9 😎 Browse the preview: https://deploy-preview-4870--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4870--docusaurus-2.netlify.app/ |
✔️ [V1] 🔨 Explore the source changes: 0953226 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-1/deploys/60b7d52848515400073f7b1a 😎 Browse the preview: https://deploy-preview-4870--docusaurus-1.netlify.app |
Size Change: +1.08 kB (0%) Total Size: 621 kB
ℹ️ View Unchanged
|
|
||
if (!isInternalCommand(commandName)) { | ||
try { | ||
await externalCommand(cli, path.resolve('.')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure but I think this does not really execute the command, it just registers it by loading plugin commands (otherwise we skip this process as an optimisation)
I think cli.parse(process.argv);
is what executes the actual external command, so it should rather be added to the try/catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw we should probably use cli.parseAsync
instead of cli.parse
Use parseAsync instead of parse if any of your action handlers are async. Returns a Promise.
This way we wouldn't need to wrap each internal command with error handling logic, and just add a single try/catch around cli.parseAsync
once.
I think with cli.parse
it's not possible to catch the command execution error (it returns a promise instead of throwing), that's why wrapCommand
was applied everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, cli.parse
actually executes command, however wrap around this call with try-catch handles error for external command only, but internal commands still needs its own wrapper 🤷♂️
Therefore, I decided to use unhandledRejection
event, which fires for all commands.
} catch (err) { | ||
console.log( | ||
chalk.red( | ||
`Running external '${commandName}' command failed.\n\n${err}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think users will understand the concept of "external command" so it's probably useless to mention it here?
Thanks Not sure it's the perfect solution yet, but at least it works in both cases so we can merge it for now |
Motivation
Related to #4844.
In fact, we have exception handler for internal commands, so I think it makes sense to do something similar for external commands.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Not yet possible
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)