-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
bug(techdocs): Internal techdocs anchor elements do not navigate correctly when Backstage is on a subpath #16682
Conversation
Signed-off-by: Aramis Sennyey <sennyeya@amazon.com>
Changed Packages
|
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/backstage/backstage/pull/16682 ⚙️ Updating now by workflow run 4611743010. What is Uffizzi? Learn more! |
Signed-off-by: Aramis Sennyey <sennyeya@amazon.com>
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.
Nice to see this solved, thanks! 🎉
Some thoughts on how we expose this through the core APIs, looks good otherwise 👍
* | ||
* @public | ||
*/ | ||
export function useNavigateUrl() { |
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.
A lot of the functionality baked in here is already implemented by the routing system. We've discussed this need before and what I suggested back then was that we add a appRootRouteRef
that references the root of the app. I think that's the leanest way to go about this that ships this need with the fewest API additions. From what I can tell it'll work to solve this use-case. We'd export that from @backstage/core-plugin-api
.
What do you think?
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 agree that this hook isn't needed and would fit better in the existing routing API but I'm not completely following. I took a look at the @backstage/core-plugin-api
routing
folder and it seems relevant but not sure how just adding appRootRouteRef
would solve this issue. It looks like most of the existing routing is meant to translate a RouteRef
into its corresponding URL not the other way around.
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.
@Rugvip wanted to follow up on this, can you explain a little bit more your thinking here?
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.
My thinking is that you'd be able to do this:
import { appRootRouteRef, useRouteRef } from `@backstage/core-plugin-api`;
function MyComponent() {
const appRootLink = useRouteRef(appRootRouteRef);
return <a href={appRootLink()}>Link to app root</a>
}
The appRootRouteRef
would simply be bound to /
in the app, which the routing system would then resolve with the appropriate base URL.
I see what you mean though, as what you need is to be able to remove that appRootLink()
from an existing URL. Tbh I feel this is too specific of a need to add to the core APIs. The appRootRouteRef
does take you halfway though, it avoids needing to read app.baseUrl
from config, which might not always be correct, the existing logic for that in the app is here. With that in place I think the rest of the logic should be contained within TechDocs for now. Alternatively we skip the appRootRouteRef
and duplicate the config reading logic in TechDocs too, but it's a lil bit more fragile.
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 took a look at implementing this and got stuck, this line appRootLink()
is resolving to just /
. I think this is working the reverse of what we need for this PR, where the useRouteRef
removes the basePath instead of returning the full URL. For React Router, it makes sense, but it makes it impossible to resolve using useRouteRef
.
For now, let's keep it in TechDocs? I added a note to try and rework it, but I don't think it's possible with the existing system.
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'm following this PR with interest because my team is also affected by this issue. We have temporarily solved it by using a redirection as a workaround, but I would like to understand the proper way to fix it. This is important for us because we want to use backstage's routing features consistently and avoid potential conflicts or errors.
I spent a few hours trying to understand how backstage's routing works. I see that the RouteRef
's paths are prefixed with basePath
by RouteResolver
.
If I understand correctly what @Rugvip meant, simply creating appRootRouteRef
, consuming it in useTechDocsReaderDom
hook, and removing the appRootLink
prefix from the document link would simplify the proposed solution.
// core-plugin-api
export const appRootRouteRef = createRouteRef({ id: 'root', });
// useTechDocsReaderDom
const appRootLink = useRouteRef(appRootRouteRef);
// postRender
const basePath = appRootLink();
const pathname = parsedUrl.pathname.startsWith(basePath)
? parsedUrl.pathname.slice(basePath.length)
: parsedUrl.pathname;
const fullPath = `${pathname}${parsedUrl.search}${parsedUrl.hash}`;
I have some questions about this approach:
- Is this the correct way to use appRootRouteRef?
- What is the difference between
createRouteRef
,createExternalRouteRef
, andcreateSubRouteRef
? - How should the ‘/’ be map to
appRootRouteRef
in the app? (It’s clear to me how it should be mocked in tests withmountedRoutes
mock)
Could you please clarify these points for me? Thanks in advance and thank you for all the work you put into maintaining backstage :)
This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution! |
Signed-off-by: Aramis Sennyey <sennyeya@amazon.com>
This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution! |
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.
Thank you, almost good to go, just a tiny tweak
plugins/techdocs/package.json
Outdated
@@ -35,6 +35,7 @@ | |||
"dependencies": { | |||
"@backstage/catalog-model": "workspace:^", | |||
"@backstage/config": "workspace:^", | |||
"@backstage/core-app-api": "workspace:^", |
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.
This needs to stay in dev deps
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.
Updated!
Signed-off-by: Aramis Sennyey <sennyeya@amazon.com>
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.
Great, thank you! 🎉
Let's
@backstage/techdocs-maintainers, will need a 🟢 from you too |
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.
Nice work, looks good to me!
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Hey, I just made a Pull Request!
Fixes #16544.
Basically, the
react-router-dom
'snavigate
method only accepts paths (without the host, origin, or anything else). We were passing it the current page's path without the possibleapp.baseUrl
sub-path removed. This work removes that sub-path when present and provides a wrapper hook that exposes this functionality for possible re-use.✔️ Checklist
Signed-off-by
line in the message. (more info)