From 2e49348062694062ecb39cc5da29ff54d15ccb65 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Wed, 1 Mar 2023 18:21:31 -0500 Subject: [PATCH 1/4] Fixing techdocs linking issue during dom link overwrites. Signed-off-by: Aramis Sennyey --- .changeset/tender-terms-return.md | 6 + packages/core-app-api/api-report.md | 3 + packages/core-app-api/src/routing/index.ts | 2 + .../src/routing/useNavigateUrl.test.tsx | 120 ++++++++++++++++++ .../src/routing/useNavigateUrl.tsx | 71 +++++++++++ plugins/techdocs/package.json | 2 +- .../TechDocsReaderPage.test.tsx | 14 +- .../TechDocsReaderPageContent/dom.tsx | 13 +- 8 files changed, 222 insertions(+), 9 deletions(-) create mode 100644 .changeset/tender-terms-return.md create mode 100644 packages/core-app-api/src/routing/useNavigateUrl.test.tsx create mode 100644 packages/core-app-api/src/routing/useNavigateUrl.tsx diff --git a/.changeset/tender-terms-return.md b/.changeset/tender-terms-return.md new file mode 100644 index 0000000000..3e215b213f --- /dev/null +++ b/.changeset/tender-terms-return.md @@ -0,0 +1,6 @@ +--- +'@backstage/core-app-api': minor +'@backstage/plugin-techdocs': patch +--- + +Fix a bug in sub-path navigation due to double addition of a sub-path if one was set up in `app.baseUrl`. diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index 37f7a98de0..266cebb1f1 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -576,6 +576,9 @@ export class UrlPatternDiscovery implements DiscoveryApi { getBaseUrl(pluginId: string): Promise; } +// @public +export function useNavigateUrl(): (to: string) => void; + // @public export class WebStorage implements StorageApi { constructor(namespace: string, errorApi: ErrorApi); diff --git a/packages/core-app-api/src/routing/index.ts b/packages/core-app-api/src/routing/index.ts index 46b5a6f9b4..008657d135 100644 --- a/packages/core-app-api/src/routing/index.ts +++ b/packages/core-app-api/src/routing/index.ts @@ -18,3 +18,5 @@ export { FlatRoutes } from './FlatRoutes'; export type { FlatRoutesProps } from './FlatRoutes'; export { FeatureFlagged } from './FeatureFlagged'; export type { FeatureFlaggedProps } from './FeatureFlagged'; + +export { useNavigateUrl } from './useNavigateUrl'; diff --git a/packages/core-app-api/src/routing/useNavigateUrl.test.tsx b/packages/core-app-api/src/routing/useNavigateUrl.test.tsx new file mode 100644 index 0000000000..383ee4d688 --- /dev/null +++ b/packages/core-app-api/src/routing/useNavigateUrl.test.tsx @@ -0,0 +1,120 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import React from 'react'; +import { + MockConfigApi, + renderInTestApp, + TestApiProvider, +} from '@backstage/test-utils'; +import { resolveUrlToRelative, useNavigateUrl } from './useNavigateUrl'; +import { configApiRef } from '@backstage/core-plugin-api'; + +const navigate = jest.fn(); +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), + useNavigate: () => navigate, +})); + +describe('resolveUrlToRelative', () => { + it('does nothing when app.baseUrl has no subpath', () => { + const url = 'http://localhost:3000/test'; + const baseUrl = 'http://localhost:3000'; + expect(resolveUrlToRelative(url, baseUrl)).toBe('/test'); + }); + + it('removes the app.baseUrl subpath when present', () => { + const url = 'http://localhost:3000/instance/test'; + const baseUrl = 'http://localhost:3000/instance'; + expect(resolveUrlToRelative(url, baseUrl)).toBe('/test'); + }); + + it('removes trailing slashes on the URL when present', () => { + const url = 'http://localhost:3000/test//'; + const baseUrl = 'http://localhost:3000'; + expect(resolveUrlToRelative(url, baseUrl)).toBe('/test'); + }); +}); + +const Component = ({ to }: { to: string }) => { + const navigateTo = useNavigateUrl(); + return <>{navigateTo(to)}; +}; + +describe('useNavigateUrl', () => { + beforeEach(() => { + navigate.mockReset(); + }); + it('navigates to the desired page as expected', async () => { + const baseUrl = 'http://localhost:3000'; + await renderInTestApp( + + + , + ); + expect(navigate).toHaveBeenCalledWith('/test'); + }); + it('handles app.baseUrl subpaths', async () => { + const baseUrl = 'http://localhost:3000/instance'; + await renderInTestApp( + + + , + ); + expect(navigate).toHaveBeenCalledWith('/test'); + }); + it('handles relative urls', async () => { + const baseUrl = 'http://localhost:3000'; + await renderInTestApp( + + + , + ); + expect(navigate).toHaveBeenCalledWith('/test'); + }); +}); diff --git a/packages/core-app-api/src/routing/useNavigateUrl.tsx b/packages/core-app-api/src/routing/useNavigateUrl.tsx new file mode 100644 index 0000000000..5e1bd42c6b --- /dev/null +++ b/packages/core-app-api/src/routing/useNavigateUrl.tsx @@ -0,0 +1,71 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { configApiRef, useApi } from '@backstage/core-plugin-api'; +import { useCallback } from 'react'; +import { useNavigate } from 'react-router-dom'; + +/** + * Resolve a URL to a relative URL given a base URL that may or may not include subpaths. + * @param url - URL to parse into a relative url based on the baseUrl. + * @param baseUrl - Application base url, where the application is currently hosted. + * @returns relative path without any subpaths from website config. + */ +export function resolveUrlToRelative(url: string, baseUrl: string) { + const parsedAppUrl = new URL(baseUrl); + const appUrlPath = `${parsedAppUrl.origin}${parsedAppUrl.pathname.replace( + /\/$/, + '', + )}`; + + const relativeUrl = url + .replace(appUrlPath, '') + // Remove any leading and trailing slashes. + .replace(/\/+$/, '') + .replace(/^\/+/, ''); + const parsedUrl = new URL(`http://localhost/${relativeUrl}`); + return `${parsedUrl.pathname}${parsedUrl.search}${parsedUrl.hash}`; +} + +/** + * A helper hook that allows for full internal website urls to be processed through the navigate + * hook provided by `react-router-dom`. + * + * NOTE: This does not support routing to external URLs. That should be done with a `Link` or `a` + * element instead, or just `window.location.href`. + * + * @returns Navigation function that is a wrapper over `react-router-dom`'s + * to support passing full URLs for navigation. + * + * @public + */ +export function useNavigateUrl() { + const navigate = useNavigate(); + const configApi = useApi(configApiRef); + const appBaseUrl = configApi.getString('app.baseUrl'); + const navigateFn = useCallback( + (to: string) => { + let url = to; + try { + url = resolveUrlToRelative(to, appBaseUrl); + } catch (err) { + // URL passed in was relative. + } + navigate(url); + }, + [navigate, appBaseUrl], + ); + return navigateFn; +} diff --git a/plugins/techdocs/package.json b/plugins/techdocs/package.json index 8f15b11152..201dc3e71a 100644 --- a/plugins/techdocs/package.json +++ b/plugins/techdocs/package.json @@ -35,6 +35,7 @@ "dependencies": { "@backstage/catalog-model": "workspace:^", "@backstage/config": "workspace:^", + "@backstage/core-app-api": "workspace:^", "@backstage/core-components": "workspace:^", "@backstage/core-plugin-api": "workspace:^", "@backstage/errors": "workspace:^", @@ -65,7 +66,6 @@ }, "devDependencies": { "@backstage/cli": "workspace:^", - "@backstage/core-app-api": "workspace:^", "@backstage/dev-utils": "workspace:^", "@backstage/plugin-techdocs-module-addons-contrib": "workspace:^", "@backstage/test-utils": "workspace:^", diff --git a/plugins/techdocs/src/reader/components/TechDocsReaderPage/TechDocsReaderPage.test.tsx b/plugins/techdocs/src/reader/components/TechDocsReaderPage/TechDocsReaderPage.test.tsx index 4e8f331c7f..e11b7b6e81 100644 --- a/plugins/techdocs/src/reader/components/TechDocsReaderPage/TechDocsReaderPage.test.tsx +++ b/plugins/techdocs/src/reader/components/TechDocsReaderPage/TechDocsReaderPage.test.tsx @@ -18,7 +18,11 @@ import { act } from '@testing-library/react'; import { scmIntegrationsApiRef } from '@backstage/integration-react'; import { entityRouteRef } from '@backstage/plugin-catalog-react'; -import { renderInTestApp, TestApiProvider } from '@backstage/test-utils'; +import { + MockConfigApi, + renderInTestApp, + TestApiProvider, +} from '@backstage/test-utils'; import { techdocsApiRef, techdocsStorageApiRef } from '../../../api'; @@ -31,6 +35,7 @@ import { ReportIssue } from '@backstage/plugin-techdocs-module-addons-contrib'; import { FlatRoutes } from '@backstage/core-app-api'; import { Page } from '@backstage/core-components'; +import { configApiRef } from '@backstage/core-plugin-api'; const mockEntityMetadata = { locationMetadata: { @@ -80,11 +85,18 @@ jest.mock('@backstage/core-components', () => ({ Page: jest.fn(), })); +const configApi = new MockConfigApi({ + app: { + baseUrl: 'http://localhost:3000', + }, +}); + const Wrapper = ({ children }: { children: React.ReactNode }) => { return ( { - const navigate = useNavigate(); + const navigate = useNavigateUrl(); const theme = useTheme(); const isMobileMedia = useMediaQuery(MOBILE_MEDIA_QUERY); const sanitizerTransformer = useSanitizerTransformer(); @@ -176,7 +176,6 @@ export const useTechDocsReaderDom = ( // detect if CTRL or META keys are pressed so that links can be opened in a new tab with `window.open` const modifierActive = event.ctrlKey || event.metaKey; const parsedUrl = new URL(url); - const fullPath = `${parsedUrl.pathname}${parsedUrl.search}${parsedUrl.hash}`; // capture link clicks within documentation const linkText = @@ -187,9 +186,9 @@ export const useTechDocsReaderDom = ( // hash exists when anchor is clicked on secondary sidebar if (parsedUrl.hash) { if (modifierActive) { - window.open(fullPath, '_blank'); + window.open(url, '_blank'); } else { - navigate(fullPath); + navigate(url); // Scroll to hash if it's on the current page transformedElement ?.querySelector(`[id="${parsedUrl.hash.slice(1)}"]`) @@ -197,9 +196,9 @@ export const useTechDocsReaderDom = ( } } else { if (modifierActive) { - window.open(fullPath, '_blank'); + window.open(url, '_blank'); } else { - navigate(fullPath); + navigate(url); } } }, From 383f30f9d6060e0436eb9a0b41e503441d7dd155 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Thu, 2 Mar 2023 16:50:42 -0500 Subject: [PATCH 2/4] Ensure backwards compatibility. Signed-off-by: Aramis Sennyey --- .../core-app-api/src/routing/useNavigateUrl.tsx | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/core-app-api/src/routing/useNavigateUrl.tsx b/packages/core-app-api/src/routing/useNavigateUrl.tsx index 5e1bd42c6b..3f067f475e 100644 --- a/packages/core-app-api/src/routing/useNavigateUrl.tsx +++ b/packages/core-app-api/src/routing/useNavigateUrl.tsx @@ -54,14 +54,20 @@ export function resolveUrlToRelative(url: string, baseUrl: string) { export function useNavigateUrl() { const navigate = useNavigate(); const configApi = useApi(configApiRef); - const appBaseUrl = configApi.getString('app.baseUrl'); + const appBaseUrl = configApi.getOptionalString('app.baseUrl'); const navigateFn = useCallback( (to: string) => { let url = to; - try { - url = resolveUrlToRelative(to, appBaseUrl); - } catch (err) { - // URL passed in was relative. + /** + * This should always be true when running the application, this just allows + * test cases that do not have the configApi set up to run still. + */ + if (appBaseUrl) { + try { + url = resolveUrlToRelative(to, appBaseUrl); + } catch (err) { + // URL passed in was relative. + } } navigate(url); }, From d31664d154ab5327975a987c8cbfec6daa978ec2 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Fri, 24 Mar 2023 12:35:15 -0400 Subject: [PATCH 3/4] Remove from public interface and make techdocs specific. Signed-off-by: Aramis Sennyey --- .changeset/tender-terms-return.md | 1 - packages/core-app-api/api-report.md | 3 --- packages/core-app-api/src/routing/index.ts | 2 -- .../src/reader/components/TechDocsReaderPageContent/dom.tsx | 2 +- .../TechDocsReaderPageContent}/useNavigateUrl.test.tsx | 0 .../components/TechDocsReaderPageContent}/useNavigateUrl.tsx | 2 ++ 6 files changed, 3 insertions(+), 7 deletions(-) rename {packages/core-app-api/src/routing => plugins/techdocs/src/reader/components/TechDocsReaderPageContent}/useNavigateUrl.test.tsx (100%) rename {packages/core-app-api/src/routing => plugins/techdocs/src/reader/components/TechDocsReaderPageContent}/useNavigateUrl.tsx (97%) diff --git a/.changeset/tender-terms-return.md b/.changeset/tender-terms-return.md index 3e215b213f..9e8ab6beb7 100644 --- a/.changeset/tender-terms-return.md +++ b/.changeset/tender-terms-return.md @@ -1,5 +1,4 @@ --- -'@backstage/core-app-api': minor '@backstage/plugin-techdocs': patch --- diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index 266cebb1f1..37f7a98de0 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -576,9 +576,6 @@ export class UrlPatternDiscovery implements DiscoveryApi { getBaseUrl(pluginId: string): Promise; } -// @public -export function useNavigateUrl(): (to: string) => void; - // @public export class WebStorage implements StorageApi { constructor(namespace: string, errorApi: ErrorApi); diff --git a/packages/core-app-api/src/routing/index.ts b/packages/core-app-api/src/routing/index.ts index 008657d135..46b5a6f9b4 100644 --- a/packages/core-app-api/src/routing/index.ts +++ b/packages/core-app-api/src/routing/index.ts @@ -18,5 +18,3 @@ export { FlatRoutes } from './FlatRoutes'; export type { FlatRoutesProps } from './FlatRoutes'; export { FeatureFlagged } from './FeatureFlagged'; export type { FeatureFlaggedProps } from './FeatureFlagged'; - -export { useNavigateUrl } from './useNavigateUrl'; diff --git a/plugins/techdocs/src/reader/components/TechDocsReaderPageContent/dom.tsx b/plugins/techdocs/src/reader/components/TechDocsReaderPageContent/dom.tsx index 092875609d..cc42c46b4e 100644 --- a/plugins/techdocs/src/reader/components/TechDocsReaderPageContent/dom.tsx +++ b/plugins/techdocs/src/reader/components/TechDocsReaderPageContent/dom.tsx @@ -46,7 +46,7 @@ import { useSanitizerTransformer, useStylesTransformer, } from '../../transformers'; -import { useNavigateUrl } from '@backstage/core-app-api'; +import { useNavigateUrl } from './useNavigateUrl'; const MOBILE_MEDIA_QUERY = 'screen and (max-width: 76.1875em)'; diff --git a/packages/core-app-api/src/routing/useNavigateUrl.test.tsx b/plugins/techdocs/src/reader/components/TechDocsReaderPageContent/useNavigateUrl.test.tsx similarity index 100% rename from packages/core-app-api/src/routing/useNavigateUrl.test.tsx rename to plugins/techdocs/src/reader/components/TechDocsReaderPageContent/useNavigateUrl.test.tsx diff --git a/packages/core-app-api/src/routing/useNavigateUrl.tsx b/plugins/techdocs/src/reader/components/TechDocsReaderPageContent/useNavigateUrl.tsx similarity index 97% rename from packages/core-app-api/src/routing/useNavigateUrl.tsx rename to plugins/techdocs/src/reader/components/TechDocsReaderPageContent/useNavigateUrl.tsx index 3f067f475e..dfe1058d75 100644 --- a/packages/core-app-api/src/routing/useNavigateUrl.tsx +++ b/plugins/techdocs/src/reader/components/TechDocsReaderPageContent/useNavigateUrl.tsx @@ -46,6 +46,8 @@ export function resolveUrlToRelative(url: string, baseUrl: string) { * NOTE: This does not support routing to external URLs. That should be done with a `Link` or `a` * element instead, or just `window.location.href`. * + * TODO: Update this to use `useRouteRef` instead of `useApi`. + * * @returns Navigation function that is a wrapper over `react-router-dom`'s * to support passing full URLs for navigation. * From 9e4198d88d360057746ee08d11315a503c652c5c Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Tue, 4 Apr 2023 15:40:00 -0400 Subject: [PATCH 4/4] Move core-app-api to dev. Signed-off-by: Aramis Sennyey --- plugins/techdocs/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/techdocs/package.json b/plugins/techdocs/package.json index 201dc3e71a..8f15b11152 100644 --- a/plugins/techdocs/package.json +++ b/plugins/techdocs/package.json @@ -35,7 +35,6 @@ "dependencies": { "@backstage/catalog-model": "workspace:^", "@backstage/config": "workspace:^", - "@backstage/core-app-api": "workspace:^", "@backstage/core-components": "workspace:^", "@backstage/core-plugin-api": "workspace:^", "@backstage/errors": "workspace:^", @@ -66,6 +65,7 @@ }, "devDependencies": { "@backstage/cli": "workspace:^", + "@backstage/core-app-api": "workspace:^", "@backstage/dev-utils": "workspace:^", "@backstage/plugin-techdocs-module-addons-contrib": "workspace:^", "@backstage/test-utils": "workspace:^",