-
-
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
feat(v2): warn user when there are conflicting routes #3083
feat(v2): warn user when there are conflicting routes #3083
Conversation
Deploy preview for docusaurus-2 ready! Built with commit 04587de |
…ikjun/overriding-routes-warning
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.
Tested and it seems to work fine.
However, can you make an implementation where detecting the bad routes is decorrelated from the effect (logging/throwing).
This way you can make the code more testable, and you'd be able to add tests easily on the most important parts.
Can you rework this PR, keeping the same behavior for now, and making the effect configurable with a onDuplicatePaths
setting or something?
You can get inspiration from here, as the broken links detection behavior is quite similar to this
https://github.com/facebook/docusaurus/pull/3059/files#diff-e6be3562ac400435b5f35aa3276065b4R1
FYI, just remember this code in docs plugin. Maybe we should remove it or this plugin won't report the docs/home conflict correctly?
|
Ah I was just thinking about this, wasn't sure if this code was still needed. Ok, I'll just remove this code in docs plugin 👌 The warning is reported before the |
This reverts commit 8b2caaa.
…ikjun/overriding-routes-warning
…ikjun/overriding-routes-warning
…overriding-routes-warning
I've completed the requested changes :D |
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.
That looks nice to me, just a few minor changes
packages/docusaurus/src/server/__tests__/duplicateRoutes.test.ts
Outdated
Show resolved
Hide resolved
return flatMap(routeConfig, getFinalRoutes); | ||
} | ||
|
||
export function reportMessage( |
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 think this could be used for reporting things other than errors, so I named it in a general way.
I also prepended the type of message (info, warn, error) in front of the body so it looks similar to the other messages that we see in the cli (I've updated the screenshot in the original post)
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 that looks fine
Ready for review! :) |
Motivation
Users may accidentally create conflicting routes. This is especially common when the user enables docs-only mode or blog-only mode. This PR adds a warning to inform the user that there are overriding paths when they run
yarn start
oryarn build
. It also adds documentation that explains the warning.This PR closes #3041.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
docusaurus.config.js
of D2 website, add a linerouteBasePath: '/',
to the docs-plugin section (line 116).yarn start
. There should be 3 warnings.