From db2d829013722957332bcf03928685a4771f9a3c Mon Sep 17 00:00:00 2001 From: Dimitris Klouvas Date: Thu, 11 Jan 2024 17:58:48 +0200 Subject: [PATCH] fix(clerk-react): Improve error message and checks for routing strategies (#2543) --- .changeset/witty-shirts-dream.md | 15 +++++ packages/react/src/errors/messages.ts | 5 +- .../useRoutingProps.test.tsx.snap | 22 ++----- .../hooks/__tests__/useRoutingProps.test.tsx | 57 +++++++++++++------ packages/react/src/hooks/useRoutingProps.ts | 23 ++++---- 5 files changed, 75 insertions(+), 47 deletions(-) create mode 100644 .changeset/witty-shirts-dream.md diff --git a/.changeset/witty-shirts-dream.md b/.changeset/witty-shirts-dream.md new file mode 100644 index 0000000000..3c37500df1 --- /dev/null +++ b/.changeset/witty-shirts-dream.md @@ -0,0 +1,15 @@ +--- +'@clerk/clerk-react': minor +--- + +Apply the following changes to components with routing props: +- default is `routing="path"` and `path` prop is required to be set via env or context +- when `routing="hash"` or `routing="virtual"` is set the implicit (via env or context) `path` option is ignored +- when `routing="hash"` or `routing="virtual"` then `path` prop is not allowed to be set + +Examples of components with routing props: +- `` +- `` +- `` +- `` +- `` diff --git a/packages/react/src/errors/messages.ts b/packages/react/src/errors/messages.ts index cfcbab1c7f..0208f7f097 100644 --- a/packages/react/src/errors/messages.ts +++ b/packages/react/src/errors/messages.ts @@ -37,4 +37,7 @@ export const useAuthHasRequiresRoleOrPermission = 'Missing parameters. `has` from `useAuth` requires a permission or role key to be passed. Example usage: `has({permission: "org:posts:edit"`'; export const noPathProvidedError = (componentName: string) => - `<${componentName}/> is missing a path prop to work with path based routing`; + `The <${componentName}/> component uses path-based routing by default unless a different routing strategy is provided using the \`routing\` prop. When path-based routing is used, you need to provide the path where the component is mounted on by using the \`path\` prop. Example: <${componentName} path={'/my-path'} />`; + +export const incompatibleRoutingWithPathProvidedError = (componentName: string) => + `The \`path\` prop will only be respected when the Clerk component uses path-based routing. Please update the <${componentName}/> component and either drop the \`path\` prop or update the routing strategy using the \`routing\` prop. For more details please refer to our docs: https://clerk.com/docs`; diff --git a/packages/react/src/hooks/__tests__/__snapshots__/useRoutingProps.test.tsx.snap b/packages/react/src/hooks/__tests__/__snapshots__/useRoutingProps.test.tsx.snap index f9d8f44246..aad50c7788 100644 --- a/packages/react/src/hooks/__tests__/__snapshots__/useRoutingProps.test.tsx.snap +++ b/packages/react/src/hooks/__tests__/__snapshots__/useRoutingProps.test.tsx.snap @@ -1,40 +1,30 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`useRoutingProps() returns passed props and options for routing different than path 1`] = ` +exports[`useRoutingProps() path prop has priority over path option 1`] = `
- {"routing":"virtual","prop1":"1","prop2":"2"} -
-
- -`; - -exports[`useRoutingProps() returns passed props and options if path is passed from routing options 1`] = ` - -
-
- {"path":"/aloha","prop1":"1","prop2":"2","routing":"path"} + {"path":"/path-prop","prop1":"1","prop2":"2","routing":"path"}
`; -exports[`useRoutingProps() returns passed props but omits path prop if path is passed from routing options and a routing strategy other than path is specified 1`] = ` +exports[`useRoutingProps() the path option is ignored when "hash" routing prop 1`] = `
- {"prop1":"1","prop2":"2","routing":"virtual"} + {"routing":"hash","prop1":"1","prop2":"2"}
`; -exports[`useRoutingProps() returns path routing with passed props and options if path prop is passed 1`] = ` +exports[`useRoutingProps() the path option is ignored when "virtual" routing prop 1`] = `
- {"path":"/aloha","prop1":"1","prop2":"2","routing":"path"} + {"routing":"virtual","prop1":"1","prop2":"2"}
diff --git a/packages/react/src/hooks/__tests__/useRoutingProps.test.tsx b/packages/react/src/hooks/__tests__/useRoutingProps.test.tsx index 2afcf9065e..f363ee03e8 100644 --- a/packages/react/src/hooks/__tests__/useRoutingProps.test.tsx +++ b/packages/react/src/hooks/__tests__/useRoutingProps.test.tsx @@ -14,7 +14,7 @@ describe('useRoutingProps()', () => { console.error = originalError; }); - it('throws error without routing & path props', () => { + it('defaults to path routing and a path prop is required', () => { const TestingComponent = props => { const options = useRoutingProps('TestingComponent', props); return
{JSON.stringify(options)}
; @@ -22,18 +22,18 @@ describe('useRoutingProps()', () => { expect(() => { render(); - }).toThrowError(' is missing a path prop to work with path based routing'); + }).toThrowError(/@clerk\/clerk-react: The component uses path-based routing by default/); }); - it('returns path routing with passed props and options if path prop is passed', () => { + it('the path option is ignored when "hash" routing prop', () => { const TestingComponent = props => { - const options = useRoutingProps('TestingComponent', props); + const options = useRoutingProps('TestingComponent', props, { path: '/path-option' }); return
{JSON.stringify(options)}
; }; const { baseElement } = render( , @@ -41,9 +41,9 @@ describe('useRoutingProps()', () => { expect(baseElement).toMatchSnapshot(); }); - it('returns passed props and options for routing different than path', () => { + it('the path option is ignored when "virtual" routing prop', () => { const TestingComponent = props => { - const options = useRoutingProps('TestingComponent', props); + const options = useRoutingProps('TestingComponent', props, { path: '/path-option' }); return
{JSON.stringify(options)}
; }; @@ -57,32 +57,53 @@ describe('useRoutingProps()', () => { expect(baseElement).toMatchSnapshot(); }); - it('returns passed props and options if path is passed from routing options', () => { + it('throws error when "hash" routing and path prop are set', () => { const TestingComponent = props => { - const options = useRoutingProps('TestingComponent', props, { path: '/aloha' }); + const options = useRoutingProps('TestingComponent', props); return
{JSON.stringify(options)}
; }; - const { baseElement } = render( - , + expect(() => { + render( + , + ); + }).toThrowError( + /@clerk\/clerk-react: The `path` prop will only be respected when the Clerk component uses path-based routing/, ); - expect(baseElement).toMatchSnapshot(); }); - it('returns passed props but omits path prop if path is passed from routing options and a routing strategy other than path is specified', () => { + it('throws error when "virtual" routing and path prop are set', () => { const TestingComponent = props => { - const options = useRoutingProps('TestingComponent', props, { path: '/aloha' }); + const options = useRoutingProps('TestingComponent', props); + return
{JSON.stringify(options)}
; + }; + + expect(() => { + render( + , + ); + }).toThrowError( + /@clerk\/clerk-react: The `path` prop will only be respected when the Clerk component uses path-based routing/, + ); + }); + + it('path prop has priority over path option', () => { + const TestingComponent = props => { + const options = useRoutingProps('TestingComponent', props, { path: '/path-option' }); return
{JSON.stringify(options)}
; }; const { baseElement } = render( , ); expect(baseElement).toMatchSnapshot(); diff --git a/packages/react/src/hooks/useRoutingProps.ts b/packages/react/src/hooks/useRoutingProps.ts index 5be7687114..8ca7c18db3 100644 --- a/packages/react/src/hooks/useRoutingProps.ts +++ b/packages/react/src/hooks/useRoutingProps.ts @@ -1,7 +1,7 @@ import type { RoutingOptions } from '@clerk/types'; import { errorThrower } from '../errors/errorThrower'; -import { noPathProvidedError } from '../errors/messages'; +import { incompatibleRoutingWithPathProvidedError, noPathProvidedError } from '../errors/messages'; export function useRoutingProps( componentName: string, @@ -9,19 +9,13 @@ export function useRoutingProps( routingOptions?: RoutingOptions, ): T { const path = props.path || routingOptions?.path; - if (!path && !props.routing) { - errorThrower.throw(noPathProvidedError(componentName)); - } + const routing = props.routing || routingOptions?.routing || 'path'; - if (props.routing && props.routing !== 'path' && routingOptions?.path && !props.path) { - return { - ...routingOptions, - ...props, - path: undefined, - }; - } + if (routing === 'path') { + if (!path) { + return errorThrower.throw(noPathProvidedError(componentName)); + } - if (path && !props.routing) { return { ...routingOptions, ...props, @@ -29,8 +23,13 @@ export function useRoutingProps( }; } + if (props.path) { + return errorThrower.throw(incompatibleRoutingWithPathProvidedError(componentName)); + } + return { ...routingOptions, ...props, + path: undefined, }; }