feat(clerk-react,remix,nextjs): Make path the default routing strategy for NextJS and Remix #2338

octoper · 2023-12-13T17:15:45Z

Description

This PR introduces changes that will make path strategy the default if the path prop is filled for Next.js and Remix frameworks, previously the routing strategy if there is no routing or path prop filled we would fallback to hash routing strategy.

In addition if no path prop is filled an error will be thrown.

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

changeset-bot · 2023-12-13T17:15:49Z

🦋 Changeset detected

Latest commit: 563f1e7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
@clerk/clerk-js	Major
@clerk/nextjs	Major
@clerk/clerk-react	Major
@clerk/remix	Major
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/elements	Patch
gatsby-plugin-clerk	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

tmilewski · 2023-12-13T22:29:08Z

@octoper I was able to unblock the CI issues, but had to pull work in from main. Seems there's still some, legit, unit tests failing. 👍

SokratisVidros · 2023-12-14T13:10:46Z

packages/react/src/components/withPathDefaultRouting.tsx

+import { errorThrower } from '../errors/errorThrower';
+import { noPathProvidedError } from '../errors/messages';
+
+export function withPathDefaultRouting<T, P extends RoutingOptions>(Component: T, componentName: string): T {


@dimkl Shall we move this to the new react/internal subpath export?

packages/react/src/errors/messages.ts

SokratisVidros · 2023-12-14T13:13:03Z

playground/nextjs/pages/organization/[[...index]].tsx

@@ -7,7 +7,7 @@ const OrganizationProfilePage: NextPage = () => {
    <div
      style={{ display: 'flex', flexDirection: 'column', gap: '2rem', justifyContent: 'center', alignItems: 'center' }}
    >
-      <OrganizationProfile />
+      <OrganizationProfile path='/organization' />


❓ Will this error on dev time due to missing type?

This error will be shown when the component is being rendered. This is a runtime error

nikosdouvlis · 2023-12-14T12:37:49Z

packages/nextjs/src/client-boundary/uiComponents.tsx

 } from '@clerk/clerk-react';

+export const UserProfile: typeof BaseUserProfile = withPathDefaultRouting(BaseUserProfile, 'UserProfile');


Good job using a HOC for this, but I feel that the types here should just work - why did you need to use typeof here?

Could it be that the type of the HOC itself needs tweaking so the type can be inferred correctly?

I started seeing an error at some point after this PR, that the types couldn't not be inferred automatically.

nikosdouvlis · 2023-12-14T13:15:08Z

packages/nextjs/src/client-boundary/uiComponents.tsx

@@ -37,6 +54,9 @@ export const SignIn = (props: SignInProps) => {
 export const SignUp = (props: SignUpProps) => {


It seems that you're not using the HOC for the components that are mounted in paths we can infer using the env variables.

I feel like we should align this behavior by either:

making the Hoc generic enough to support both cases

supported env variables for all components

Do you want to discuss offline for possible solutions? :)

Yep I thought about that too, but the path of <SignIn/> and <SignUp/> can be also defined from the Clerk options on the <ClerkProvider/> and I didn't know if we wanted to make the changes to have ENV and options for every component.

dimkl · 2023-12-18T17:25:08Z

.changeset/good-buttons-drum.md

+<OrganizationProfile routing="hash_or_virtual"/>
+<SignIn routing="hash_or_virtual"/>
+<SignUp routing="hash_or_virtual"/>
+```


@SokratisVidros , @nikosdouvlis i have also added examples in changeset

…useRoutingOptions

This change is applied to show correct component name. Using Component.displayName was causing a `<withClerk(Component)/>` to be shown in error message

clerk/javascript#2543 clerk/javascript#2338

clerk-cookie added nextjs react remix playground labels Dec 13, 2023

octoper marked this pull request as ready for review December 13, 2023 18:05

tmilewski force-pushed the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch 2 times, most recently from a5ca7a4 to d2d8af4 Compare December 13, 2023 22:06

octoper force-pushed the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch from d2d8af4 to 25ff3f5 Compare December 13, 2023 23:09

octoper requested review from dimkl, tmilewski and SokratisVidros December 13, 2023 23:28

octoper force-pushed the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch from 25ff3f5 to 414cbd6 Compare December 14, 2023 07:57

clerk-cookie added the chrome-extension label Dec 14, 2023

octoper force-pushed the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch 2 times, most recently from fdad171 to 0327a81 Compare December 14, 2023 08:43

SokratisVidros reviewed Dec 14, 2023

View reviewed changes

nikosdouvlis reviewed Dec 14, 2023

View reviewed changes

dimkl self-assigned this Dec 15, 2023

dimkl added the v5-release label Dec 15, 2023

dimkl force-pushed the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch from cfe4ca0 to 4da9391 Compare December 18, 2023 16:47

clerk-cookie removed the chrome-extension label Dec 18, 2023

dimkl force-pushed the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch from 460354b to 0a122e4 Compare December 18, 2023 17:12

dimkl requested review from nikosdouvlis and SokratisVidros December 18, 2023 17:19

dimkl force-pushed the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch from 6067887 to c62c439 Compare December 18, 2023 17:24

dimkl reviewed Dec 18, 2023

View reviewed changes

dimkl force-pushed the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch from c62c439 to 1922a1d Compare December 18, 2023 18:11

clerk-cookie added the integration label Dec 18, 2023

octoper and others added 9 commits December 19, 2023 11:04

feat(clerk-react,remix,nextjs): Default routing for NextJS and Remix

85a5f3e

chore(repo): Update changeset

97a1ea6

chore(repo): Update NextJs playground

cd8ae3a

fix(nextjs,remix): Throw error if no path or routing strategy provided

7ca360e

fix(nextjs,remix,clerk-react): Improve error for withPathDefaultRouting

24c375a

chore(remix,nextjs,clerk-react): Replace withPathDefaultRouting with …

f82b75b

…useRoutingOptions

chore(remix,nextjs): Use hard-coded component name

36fa16d

This change is applied to show correct component name. Using Component.displayName was causing a `<withClerk(Component)/>` to be shown in error message

chore(repo): Update changeset with examples

b41d612

chore(repo): Disable telemetry in integration tests

563f1e7

dimkl force-pushed the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch from 1922a1d to 563f1e7 Compare December 19, 2023 09:04

SokratisVidros approved these changes Dec 19, 2023

View reviewed changes

nikosdouvlis approved these changes Dec 19, 2023

View reviewed changes

dimkl added this pull request to the merge queue Dec 19, 2023

Merged via the queue into main with commit cfea3d9 Dec 19, 2023
7 checks passed

dimkl deleted the vaggelis/sdk-663-make-path-based-routing-the-default-for-frameworks-that branch December 19, 2023 10:56

clerk-cookie mentioned this pull request Dec 19, 2023

Version Packages (alpha-v5) #2397

Merged

dimkl added a commit to clerk/clerk-docs that referenced this pull request Jan 15, 2024

fix(v5-upgrade-guide): Add routing strategy changes

a1ee014

clerk/javascript#2543 clerk/javascript#2338

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clerk-react,remix,nextjs): Make path the default routing strategy for NextJS and Remix #2338

feat(clerk-react,remix,nextjs): Make path the default routing strategy for NextJS and Remix #2338

octoper commented Dec 13, 2023 •

edited

Loading

changeset-bot bot commented Dec 13, 2023 •

edited

Loading

tmilewski commented Dec 13, 2023

SokratisVidros Dec 14, 2023

SokratisVidros Dec 14, 2023

dimkl Dec 18, 2023

nikosdouvlis Dec 14, 2023

octoper Dec 14, 2023

nikosdouvlis Dec 14, 2023

SokratisVidros Dec 14, 2023

octoper Dec 14, 2023

dimkl Dec 18, 2023

		} from '@clerk/clerk-react';

		export const UserProfile: typeof BaseUserProfile = withPathDefaultRouting(BaseUserProfile, 'UserProfile');

		@@ -37,6 +54,9 @@ export const SignIn = (props: SignInProps) => {
		export const SignUp = (props: SignUpProps) => {

feat(clerk-react,remix,nextjs): Make path the default routing strategy for NextJS and Remix #2338

feat(clerk-react,remix,nextjs): Make path the default routing strategy for NextJS and Remix #2338

Conversation

octoper commented Dec 13, 2023 • edited Loading

Description

Checklist

Type of change

Packages affected

changeset-bot bot commented Dec 13, 2023 • edited Loading

🦋 Changeset detected

tmilewski commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octoper commented Dec 13, 2023 •

edited

Loading

changeset-bot bot commented Dec 13, 2023 •

edited

Loading