fix(clerk-react): Improve error message and checks for routing strate…

…gies (#2543)
clerk · Jan 11, 2024 · db2d829 · db2d829
1 parent 799abc2
commit db2d829
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 47 deletions.
diff --git a/.changeset/witty-shirts-dream.md 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:
+- `<CreateOrganization />`
+- `<OrganizationProfile />`
+- `<SignIn />`
+- `<SignUp />`
+- `<UserProfile />`
diff --git 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
@@ -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`] = `
 <body>
   <div>
     <div>
-      {"routing":"virtual","prop1":"1","prop2":"2"}
-    </div>
-  </div>
-</body>
-`;
-
-exports[`useRoutingProps() returns passed props and options if path is passed from routing options 1`] = `
-<body>
-  <div>
-    <div>
-      {"path":"/aloha","prop1":"1","prop2":"2","routing":"path"}
+      {"path":"/path-prop","prop1":"1","prop2":"2","routing":"path"}
     </div>
   </div>
 </body>
 `;
 
-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`] = `
 <body>
   <div>
     <div>
-      {"prop1":"1","prop2":"2","routing":"virtual"}
+      {"routing":"hash","prop1":"1","prop2":"2"}
     </div>
   </div>
 </body>
 `;
 
-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`] = `
 <body>
   <div>
     <div>
-      {"path":"/aloha","prop1":"1","prop2":"2","routing":"path"}
+      {"routing":"virtual","prop1":"1","prop2":"2"}
     </div>
   </div>
 </body>

diff --git a/packages/react/src/hooks/__tests__/useRoutingProps.test.tsx b/packages/react/src/hooks/__tests__/useRoutingProps.test.tsx
@@ -14,36 +14,36 @@ 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 <div>{JSON.stringify(options)}</div>;
     };
 
     expect(() => {
       render(<TestingComponent />);
-    }).toThrowError('<TestingComponent/> is missing a path prop to work with path based routing');
+    }).toThrowError(/@clerk\/clerk-react: The <TestingComponent\/> 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 <div>{JSON.stringify(options)}</div>;
     };
 
     const { baseElement } = render(
       <TestingComponent
-        path='/aloha'
+        routing='hash'
         prop1='1'
         prop2='2'
       />,
     );
     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 <div>{JSON.stringify(options)}</div>;
     };
 
@@ -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 <div>{JSON.stringify(options)}</div>;
     };
 
-    const { baseElement } = render(
-      <TestingComponent
-        prop1='1'
-        prop2='2'
-      />,
+    expect(() => {
+      render(
+        <TestingComponent
+          routing='hash'
+          path='/path-prop'
+        />,
+      );
+    }).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 <div>{JSON.stringify(options)}</div>;
+    };
+
+    expect(() => {
+      render(
+        <TestingComponent
+          routing='virtual'
+          path='/path'
+        />,
+      );
+    }).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 <div>{JSON.stringify(options)}</div>;
     };
 
     const { baseElement } = render(
       <TestingComponent
+        path='/path-prop'
         prop1='1'
         prop2='2'
-        routing='virtual'
       />,
     );
     expect(baseElement).toMatchSnapshot();

diff --git a/packages/react/src/hooks/useRoutingProps.ts b/packages/react/src/hooks/useRoutingProps.ts
@@ -1,36 +1,35 @@
 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<T extends RoutingOptions>(
   componentName: string,
   props: T,
   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,
       routing: 'path',
     };
   }
 
+  if (props.path) {
+    return errorThrower.throw(incompatibleRoutingWithPathProvidedError(componentName));
+  }
+
   return {
     ...routingOptions,
     ...props,
+    path: undefined,
   };
 }