diff --git a/.changeset/weak-oranges-drive.md b/.changeset/weak-oranges-drive.md new file mode 100644 index 0000000000..7d6828ef47 --- /dev/null +++ b/.changeset/weak-oranges-drive.md @@ -0,0 +1,7 @@ +--- +'@backstage/plugin-catalog-react': patch +--- + +Add caching to the useEntityPermission hook + +The hook now caches the authorization decision based on the permission + the entity, and returns the cache match value as the default `allowed` value while loading. This helps avoid flicker in UI elements that would be conditionally rendered based on the `allowed` result of this hook. diff --git a/plugins/permission-react/package.json b/plugins/permission-react/package.json index e4ae819247..08de9d0b4e 100644 --- a/plugins/permission-react/package.json +++ b/plugins/permission-react/package.json @@ -32,7 +32,8 @@ "@backstage/plugin-permission-common": "^0.4.0", "cross-fetch": "^3.0.6", "react-router": "6.0.0-beta.0", - "react-use": "^17.2.4" + "react-use": "^17.2.4", + "swr": "^1.1.2" }, "peerDependencies": { "@types/react": "^16.13.1 || ^17.0.0", diff --git a/plugins/permission-react/src/hooks/usePermission.test.tsx b/plugins/permission-react/src/hooks/usePermission.test.tsx index 4cf0bc6530..d6c5b2ed33 100644 --- a/plugins/permission-react/src/hooks/usePermission.test.tsx +++ b/plugins/permission-react/src/hooks/usePermission.test.tsx @@ -19,9 +19,8 @@ import { render } from '@testing-library/react'; import { usePermission } from './usePermission'; import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { TestApiProvider } from '@backstage/test-utils'; -import { permissionApiRef } from '../apis'; - -const mockAuthorize = jest.fn(); +import { PermissionApi, permissionApiRef } from '../apis'; +import { SWRConfig } from 'swr'; const permission = { name: 'access.something', @@ -39,49 +38,48 @@ const TestComponent: FC = () => { ); }; -describe('usePermission', () => { - it('Returns loading when permissionApi has not yet responded.', () => { - mockAuthorize.mockReturnValueOnce(new Promise(() => {})); - - const { getByText } = render( - +function renderComponent(mockApi: PermissionApi) { + return render( + new Map() }}> + - , - ); + + , + , + ); +} - expect(mockAuthorize).toHaveBeenCalledWith({ permission }); +describe('usePermission', () => { + const mockPermissionApi = { authorize: jest.fn() }; + + it('Returns loading when permissionApi has not yet responded.', () => { + mockPermissionApi.authorize.mockReturnValueOnce(new Promise(() => {})); + + const { getByText } = renderComponent(mockPermissionApi); + + expect(mockPermissionApi.authorize).toHaveBeenCalledWith({ permission }); expect(getByText('loading')).toBeTruthy(); }); it('Returns allowed when permissionApi allows authorization.', async () => { - mockAuthorize.mockResolvedValueOnce({ result: AuthorizeResult.ALLOW }); + mockPermissionApi.authorize.mockResolvedValueOnce({ + result: AuthorizeResult.ALLOW, + }); - const { findByText } = render( - - - , - ); + const { findByText } = renderComponent(mockPermissionApi); - expect(mockAuthorize).toHaveBeenCalledWith({ permission }); + expect(mockPermissionApi.authorize).toHaveBeenCalledWith({ permission }); expect(await findByText('content')).toBeTruthy(); }); it('Returns not allowed when permissionApi denies authorization.', async () => { - mockAuthorize.mockResolvedValueOnce({ result: AuthorizeResult.DENY }); + mockPermissionApi.authorize.mockResolvedValueOnce({ + result: AuthorizeResult.DENY, + }); - const { findByText } = render( - - - , - ); + const { findByText } = renderComponent(mockPermissionApi); - expect(mockAuthorize).toHaveBeenCalledWith({ permission }); + expect(mockPermissionApi.authorize).toHaveBeenCalledWith({ permission }); await expect(findByText('content')).rejects.toThrowError(); }); }); diff --git a/plugins/permission-react/src/hooks/usePermission.ts b/plugins/permission-react/src/hooks/usePermission.ts index 45136d6697..cc27f82c59 100644 --- a/plugins/permission-react/src/hooks/usePermission.ts +++ b/plugins/permission-react/src/hooks/usePermission.ts @@ -14,13 +14,13 @@ * limitations under the License. */ -import useAsync from 'react-use/lib/useAsync'; import { useApi } from '@backstage/core-plugin-api'; import { permissionApiRef } from '../apis'; import { AuthorizeResult, Permission, } from '@backstage/plugin-permission-common'; +import useSWR from 'swr'; /** @public */ export type AsyncPermissionResult = { @@ -30,9 +30,16 @@ export type AsyncPermissionResult = { }; /** - * React hook utlity for authorization. Given a {@link @backstage/plugin-permission-common#Permission} and an optional - * resourceRef, it will return whether or not access is allowed (for the given resource, if resourceRef is provided). See - * {@link @backstage/plugin-permission-common/PermissionClient#authorize} for more details. + * React hook utility for authorization. Given a + * {@link @backstage/plugin-permission-common#Permission} and an optional + * resourceRef, it will return whether or not access is allowed (for the given + * resource, if resourceRef is provided). See + * {@link @backstage/plugin-permission-common/PermissionClient#authorize} for + * more details. + * + * Note: This hook uses stale-while-revalidate to help avoid flicker in UI + * elements that would be conditionally rendered based on the `allowed` result + * of this hook. * @public */ export const usePermission = ( @@ -40,21 +47,16 @@ export const usePermission = ( resourceRef?: string, ): AsyncPermissionResult => { const permissionApi = useApi(permissionApiRef); - - const { loading, error, value } = useAsync(async () => { - const { result } = await permissionApi.authorize({ - permission, - resourceRef, - }); - + const { data, error } = useSWR({ permission, resourceRef }, async args => { + const { result } = await permissionApi.authorize(args); return result; - }, [permissionApi, permission, resourceRef]); + }); - if (loading) { - return { loading: true, allowed: false }; - } if (error) { return { error, loading: false, allowed: false }; } - return { loading: false, allowed: value === AuthorizeResult.ALLOW }; + if (data === undefined) { + return { loading: true, allowed: false }; + } + return { loading: false, allowed: data === AuthorizeResult.ALLOW }; }; diff --git a/yarn.lock b/yarn.lock index 65307c3d71..780c3539d8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -23027,6 +23027,11 @@ swap-case@^2.0.2: dependencies: tslib "^2.0.3" +swr@^1.1.2: + version "1.1.2" + resolved "https://registry.npmjs.org/swr/-/swr-1.1.2.tgz#9f3de2541931fccf03c48f322f1fc935a7551612" + integrity sha512-UsM0eo5T+kRPyWFZtWRx2XR5qzohs/LS4lDC0GCyLpCYFmsfTk28UCVDbOE9+KtoXY4FnwHYiF+ZYEU3hnJ1lQ== + symbol-observable@1.2.0, symbol-observable@^1.0.4, symbol-observable@^1.1.0, symbol-observable@^1.2.0: version "1.2.0" resolved "https://registry.npmjs.org/symbol-observable/-/symbol-observable-1.2.0.tgz#c22688aed4eab3cdc2dfeacbb561660560a00804"