plugin-client-redirects error should return non-zero code on build

[matevz]

Have you read the Contributing Guidelines on issues?

• I have read the Contributing Guidelines on issues.

Prerequisites

• I'm using the latest version of Docusaurus.
• I have tried the npm run clear or yarn clear command.
• I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
• I have tried creating a repro with https://new.docusaurus.io.
• I have read the console error message carefully (if applicable).

Description

When an error in plugin-client-redirects occurs, it is only printed to std error while yarn build returns successfully:

docusaurus/packages/docusaurus-plugin-client-redirects/src/collectRedirects.ts

Line 119 in fb3138d

logger.error`name=${'@docusaurus/plugin-client-redirects'}: some redirects would override existing paths, and will be ignored: ${redirectsOverridingExistingPath.map(

This will cause CI not to fail in case of a duplicate route or other redirection errors.

Reproducible demo

No response

Steps to reproduce

1. make a docs page
2. add a duplicate route with plugin-client-redirects plugin
3. run yarn build
4. an error like this is printed to std err:

   [ERROR] @docusaurus/plugin-client-redirects: some redirects would override existing paths, and will be ignored: 
   - {"from":"/general/rfcs/","to":"/rfcs/"}
   

5. however, the return code is still 0 which causes CI to succeed even if there are errors reported:

   $ echo $?
   0

   Setting onDuplicateRoutes: 'error' doesn't help.

Expected behavior

If the error is reported, yarn build should return a non-zero code. Ideally, onDuplicateRoutes option should be considered, so if it is set to warning or less strict, yarn build still returns 0.

Actual behavior

yarn build returns 0 even if errors are reported by plugin-client-redirects.

Your environment

• Public source code: /
• Public site URL: /
• Docusaurus version used: 2.0.0-beta.21
• Environment name and version: Node.js 14.17
• Operating system and version: Ubuntu 22.04

Self-service

• I'd be willing to fix this bug myself.

[Josh-Cena]

Indeed, we simply don't have a toggle for that. Maybe we should.

[slorber]

👍 an option onDuplicateRoutes could be useful

However we already have a "throw" level. That seems more appropriate if you want to make the build fail?

We have other cases (broken links) where we can log errors and do not return a 1 exit code. To get a fail fast behavior you use "throw" and not "error".

Do we want to change how we handle "error"? or "throw" matches your intent?

[Josh-Cena]

onDuplicateRoutes is already a thing. However, we don't read that option here.

On a tangent, "error" is a quite confusing config. We should just have "throw" (terminate) / "warn" (stderr) / "log" (stdout) / "ignore"

[slorber]

onDuplicateRoutes is already a thing. However, we don't read that option here.

Ah 😅 forgot about this one. So what a good config name could we have for that redirect plugin?

On a tangent, "error" is a quite confusing config. We should just have "throw" (terminate) / "warn" (stderr) / "log" (stdout) / "ignore"

hmmm yes, should we kill "throw" or "error"? Maybe users are more used to "error" to terminate? 🤷‍♂️

[Josh-Cena]

So what a good config name could we have for that redirect plugin?

Shall we just reuse it? The idea is basically the same here: trying to output duplicate files at the same location.

should we kill "throw" or "error"?

IMO killing "error" would be less trouble for the user, because "throw" is in the init template, and I doubt if anyone uses "error" in practice. (Unless they get confused.)

[matevz]

Thanks guys!