-
Notifications
You must be signed in to change notification settings - Fork 460
feat(elements): Infer base path #3557
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
Changes from all commits
eb31e19
0455851
b746b9a
866dbe5
717bac8
d3014e5
20c43ae
eb033ea
3567e14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| '@clerk/elements': minor | ||
| --- | ||
|
|
||
| The `path` prop on the `<SignIn.Root>` and `<SignUp.Root>` component is now automatically inferred. Previously, the default values were `/sign-in` and `/sign-up`, on other routes you had to explicitly define your route. | ||
|
|
||
| The new heuristic for determining the path where `<SignIn.Root>` and `<SignUp.Root>` are mounted is: | ||
|
|
||
| 1. `path` prop | ||
| 2. Automatically inferred | ||
| 3. If it can't be inferred, fallback to `CLERK_SIGN_IN_URL` and `CLERK_SIGN_UP_URL` env var | ||
| 4. Fallback to `/sign-in` and `/sign-up` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { Router, useClerkRouter, useNextRouter, useVirtualRouter } from '~/react | |
| import { SignInRouterCtx } from '~/react/sign-in/context'; | ||
|
|
||
| import { Form } from '../common/form'; | ||
| import { usePathnameWithoutCatchAll } from '../utils/path-inference/next'; | ||
|
|
||
| type SignInFlowProviderProps = { | ||
| children: React.ReactNode; | ||
|
|
@@ -62,22 +63,29 @@ function SignInFlowProvider({ children, exampleMode }: SignInFlowProviderProps) | |
| } | ||
|
|
||
| export type SignInRootProps = SignInFlowProviderProps & { | ||
| /** | ||
| * Fallback markup to render while Clerk is loading | ||
| */ | ||
| fallback?: React.ReactNode; | ||
| /** | ||
| * The base path for your sign-in route. Defaults to `/sign-in`. | ||
| * | ||
| * TODO: re-use usePathnameWithoutCatchAll from the next SDK | ||
| * The base path for your sign-in route. | ||
| * Will be automatically inferred in Next.js. | ||
| * @example `/sign-in` | ||
| */ | ||
| path?: string; | ||
| /** | ||
| * If you want to render Clerk Elements in e.g. a modal, use the `virtual` routing mode. | ||
| */ | ||
| routing?: ROUTING; | ||
| }; | ||
|
|
||
| /** | ||
| * Root component for the sign-in flow. It sets up providers and state management for its children. | ||
| * Must wrap all sign-in related components. | ||
| * | ||
| * @param {string} path - The root path the sign-in flow is mounted at. Default: `/sign-in` | ||
| * @param {string} path - The root path the sign-in flow is mounted at. Will be automatically inferred in Next.js. You can set it to `/sign-in` for example. | ||
| * @param {React.ReactNode} fallback - Fallback markup to render while Clerk is loading. Default: `null` | ||
| * @param {string} routing - If you want to render Clerk Elements in e.g. a modal, use the `'virtual'` routing mode. Default: `'path'` | ||
| * | ||
| * @example | ||
| * import * as SignIn from "@clerk/elements/sign-in" | ||
|
|
@@ -89,18 +97,21 @@ export type SignInRootProps = SignInFlowProviderProps & { | |
| */ | ||
| export function SignInRoot({ | ||
| children, | ||
| exampleMode, | ||
| exampleMode = false, | ||
| fallback = null, | ||
| path = SIGN_IN_DEFAULT_BASE_PATH, | ||
| routing, | ||
| path: pathProp, | ||
| routing = ROUTING.path, | ||
|
Comment on lines
+100
to
+103
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made the falsy |
||
| }: SignInRootProps): JSX.Element | null { | ||
| const clerk = useClerk(); | ||
| const inferredPath = usePathnameWithoutCatchAll(); | ||
| const path = pathProp || inferredPath || SIGN_IN_DEFAULT_BASE_PATH; | ||
|
|
||
| clerk.telemetry?.record( | ||
| eventComponentMounted('Elements_SignInRoot', { | ||
| exampleMode: Boolean(exampleMode), | ||
| exampleMode, | ||
| fallback: Boolean(fallback), | ||
| path, | ||
| routing, | ||
| }), | ||
| ); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| import { renderHook } from '@testing-library/react'; | ||
| import { useRouter } from 'next/compat/router'; | ||
| import { useParams, usePathname } from 'next/navigation'; | ||
|
|
||
| import { usePathnameWithoutCatchAll } from '../next'; | ||
|
|
||
| jest.mock('next/compat/router', () => ({ | ||
| useRouter: jest.fn(), | ||
| })); | ||
| jest.mock('next/navigation', () => ({ | ||
| usePathname: jest.fn(), | ||
| useParams: jest.fn(), | ||
| })); | ||
|
|
||
| describe('usePathnameWithoutCatchAll', () => { | ||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('should work with pages router', () => { | ||
| const pathname = '/user/123/profile/[[...rest]]/page'; | ||
| const expectedPath = '/user/123/profile'; | ||
|
|
||
| (useRouter as jest.Mock).mockImplementation(() => ({ | ||
| push: jest.fn(), | ||
| pathname: pathname, | ||
| route: '/', | ||
| asPath: '/', | ||
| query: '', | ||
| })); | ||
|
|
||
| const { result } = renderHook(() => usePathnameWithoutCatchAll()); | ||
|
|
||
| expect(result.current).toBe(expectedPath); | ||
| }); | ||
|
|
||
| it('should work with app router', () => { | ||
| const pathname = '/user/123/profile/security'; | ||
| const expectedPath = '/user/123/profile'; | ||
|
|
||
| (useRouter as jest.Mock).mockImplementation(() => null); | ||
| (usePathname as jest.Mock).mockReturnValue(pathname); | ||
| (useParams as jest.Mock).mockReturnValue({ id: '123', rest: ['security'] }); | ||
|
|
||
| const { result } = renderHook(() => usePathnameWithoutCatchAll()); | ||
|
|
||
| expect(result.current).toBe(expectedPath); | ||
| }); | ||
|
|
||
| it('should work with multiple catch all params (app router)', () => { | ||
| const pathname = '/user/123/profile/security/otp'; | ||
| const expectedPath = '/user/123/profile'; | ||
|
|
||
| (useRouter as jest.Mock).mockImplementation(() => null); | ||
| (usePathname as jest.Mock).mockReturnValue(pathname); | ||
| (useParams as jest.Mock).mockReturnValue({ id: '123', rest: ['security', 'otp'] }); | ||
|
|
||
| const { result } = renderHook(() => usePathnameWithoutCatchAll()); | ||
|
|
||
| expect(result.current).toBe(expectedPath); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { removeOptionalCatchAllSegment } from '../utils'; | ||
|
|
||
| describe('removeOptionalCatchAllSegment', () => { | ||
| it('should remove optional catch-all segment from the pathname', () => { | ||
| const pathname = '/users/[[...slug]].js'; | ||
| const expected = '/users'; | ||
| expect(removeOptionalCatchAllSegment(pathname)).toEqual(expected); | ||
| }); | ||
|
|
||
| it('should not remove anything if there is no optional catch-all segment', () => { | ||
| const pathname = '/users/john/profile'; | ||
| expect(removeOptionalCatchAllSegment(pathname)).toEqual(pathname); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import { useRouter } from 'next/compat/router'; | ||
| import { useParams, usePathname } from 'next/navigation'; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in I found this papertrail:
So I think we're good with importing it directly since the version that got introduced is below our minimum Next.js version
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, does that mean we can update the one within the nextjs package as well ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, probably. But it works in that setup so "don't change a running system" |
||
| import React from 'react'; | ||
|
|
||
| import { removeOptionalCatchAllSegment } from './utils'; | ||
|
|
||
| // Adapted from packages/nextjs/src/client-boundary/hooks/usePathnameWithoutCatchAll.tsx | ||
|
|
||
| /** | ||
| * This hook grabs the current pathname (both in pages and app router) and removes any (optional) catch all segments. | ||
| * @example | ||
| * 1. /user/[id]/profile/[[...rest]]/page.tsx | ||
| * 2. /user/123/profile/security | ||
| * 3. /user/123/profile | ||
| * @returns The pathname without any catch all segments | ||
| */ | ||
| export const usePathnameWithoutCatchAll = () => { | ||
| const pathRef = React.useRef<string>(); | ||
|
|
||
| /** | ||
| * The compat version of useRouter returns null instead of throwing an error when used inside App router. | ||
| * Use it to detect if the component is used inside pages or app router | ||
| */ | ||
| const pagesRouter = useRouter(); | ||
|
|
||
| if (pagesRouter) { | ||
| if (pathRef.current) { | ||
| return pathRef.current; | ||
| } else { | ||
| // The optional catch all route is included in the pathname in pages router. It starts with [[... and we can just remove it | ||
| pathRef.current = removeOptionalCatchAllSegment(pagesRouter.pathname); | ||
| return pathRef.current; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get the pathname that includes any named or catch all params. | ||
| * @example | ||
| * /user/[id]/profile/[[...rest]]/page.tsx | ||
| * | ||
| * This filesystem route could give us the following pathname: | ||
| * /user/123/profile/security | ||
| * if the user navigates to the security section of the user profile | ||
| */ | ||
| const pathname = usePathname() || ''; | ||
| const pathParts = pathname.split('/').filter(Boolean); | ||
| /** | ||
| * For /user/[id]/profile/[[...rest]]/page.tsx useParams will return { id: '123', rest: ['security'] }. | ||
| * So catch all params are always arrays | ||
| */ | ||
| const catchAllParams = Object.values(useParams() || {}) | ||
| .filter(v => Array.isArray(v)) | ||
| .flat(Infinity); | ||
| if (pathRef.current) { | ||
| return pathRef.current; | ||
| } else { | ||
| // /user/123/profile/security should be transformed to /user/123/profile | ||
| pathRef.current = `/${pathParts.slice(0, pathParts.length - catchAllParams.length).join('/')}`; | ||
| return pathRef.current; | ||
| } | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export function removeOptionalCatchAllSegment(pathname: string) { | ||
| return pathname.replace(/\/\[\[\.\.\..*/, ''); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ export default defineConfig(overrideOptions => { | |
| 'react/sign-in/index': 'src/react/sign-in/index.ts', | ||
| 'react/sign-up/index': 'src/react/sign-up/index.ts', | ||
| }, | ||
| external: ['react', 'react-dom', '@statelyai/inspect'], | ||
| external: ['react', 'react-dom', 'next', '@statelyai/inspect'], | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While https://tsup.egoist.dev/#excluding-packages explains
I guess it doesn't hurt explicitly adding it here if we have |
||
| format: ['cjs', 'esm'], | ||
| minify: false, | ||
| sourcemap: true, | ||
|
|
||
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.
In the future we can adjust this line e.g. with Remix once we support this in other frameworks, too