Upgrade to Core 2 - Githubissues

wobsoriano commented 3 months ago

This PR upgrades the package to Clerk Core 2 with refreshed UI components and removes the @clerk/clerk-js dependency in favor of hot loading the clerk-js script which greatly reduces app bundle size. It follows the SDK development guide for Clerk.

It introduces a breaking change by removing the separate Clerk headless import and passing it as an option instead. Also, instead of the initializeClerkClient hook that is imported inside hook.client.ts, this introduces a <ClerkProvider> component to be used in the root layout and accepts the same props as the <ClerkProvider> React component :

<script lang="ts">
    import type { LayoutData } from './$types';
    import ClerkProvider from 'clerk-sveltekit/client/ClerkProvider.svelte'
    import { PUBLIC_CLERK_PUBLISHABLE_KEY } from '$env/static/public'
    export let data: LayoutData
</script>
<!-- ... -->
<ClerkProvider {...data} publishableKey={PUBLIC_CLERK_PUBLISHABLE_KEY}>
    <slot />
</ClerkProvider>

The example above provides initial auth object support in SSR, which can't be done with the initializeClerkClient function inside a client hook file.

Furthermore, it exports a useClerkContext() function that returns the ff. stores to access various Clerk resources:

clerk - clerk instance
isLoaded - clerk is loaded
client - client handling most Clerk operations
session - access to the current user's Session object
user - access the current User data
organization - access the active organization

<script>
    import { useClerkContext } from 'clerk-sveltekit/client'
    const { user, organization } = useClerkContext()
    $: console.log('User: ', $user?.fullName)
    $: console.log('Organization: ', $organization?.name)
</script>

For server, authenticateRequest is implemented, and the Auth object is injected to locals.auth:

In hooks.server.ts:

import { withClerkHandler } from 'clerk-sveltekit/server'

export const handle = withClerkHandler()

import { redirect } from '@sveltejs/kit';
import { clerkClient } from 'clerk-sveltekit/server';

export const load = ({ locals }) => {
    if (!locals.auth.userId) {
        return redirect(307, '/sign-in');
    }

    const user = await clerkClient.users.getUser(userId);

    return {
        user: JSON.parse(JSON.stringify(user))
    };
};

This is another breaking change since we're removing the protectedRoutes option and let userland handle it.

Fixes

@markjaquith I think you're actively working on this as well so feel free to ignore this. Let me know if you like this idea then I'll continue updating! Thanks :)

7Kronos commented 2 months ago

Hi any status update on this one ? Maybe we can help ?

wobsoriano commented 2 months ago

@7Kronos any help would be much appreciated! Today I implemented new server hooks.

Mark is busy at work atm btw.

whosdustin commented 2 months ago

@wobsoriano what help would you need to get this draft into a mergeable reality?

wobsoriano commented 2 months ago

@whosdustin it is mergeable now 😀 But we still have to wait til Mark is available. Busy at work atm.

I think right now, I need help finding the issue why it’s not respecting the redirect_url after signing in

whosdustin commented 2 months ago

I think right now, I need help finding the issue why it’s not respecting the redirect_url after signing in

@wobsoriano I'll try to help where I can.

markjaquith commented 2 months ago

Hey! Just checking in to see the progress on this. Have seen a lot of activity so have been waiting for it to settle and be marked as ready for review.

wobsoriano commented 2 months ago

Hey @markjaquith! Will try and fix the tests today and marked it as ready for review.

wobsoriano commented 2 months ago

Hi @markjaquith, marked as ready for review! Just FYI

The handler accepts options from authenticateRequest. They will have to manually protect their routes by checking the auth local.
I believe there's a built-in way in Clerk to handle ?redirect_url=, I'll have to check it internally then I can open another PR for it.

natehouk commented 2 months ago

Can you clarify what you mean by "removing protected routes and letting userland handle it"? Has this been resolved in the new Core 2 version? In the current version, managing route protection in userland is insecure.

natehouk commented 2 months ago

I think right now, I need help finding the issue why it’s not respecting the redirect_url after signing in

@wobsoriano I'll try to help where I can.

Any update on this redirect issue? I am also experiencing the issue.

wobsoriano commented 2 months ago

Can you clarify what you mean by "removing protected routes and letting userland handle it"? Has this been resolved in the new Core 2 version? In the current version, managing route protection in userland is insecure.

Sorry if I'm not clear with this. What I mean by that is that the old options for the server handler is removed and we (users) should be handling authorization ourselves. An example:

import { sequence } from '@sveltejs/kit/hooks'
import { withClerkHandler } from 'clerk-sveltekit/server'

export const handle = sequence(
    withClerkHandler(),
    ({ event, resolve }) => {
        const { userId } = event.locals.auth

        if (!userId && event.url.pathname.startsWith('/admin')) {
            return redirect(307, '/sign-in')
        }

        return resolve(event)
    }
)

But this only protect your routes on initial page load. Once client side routing takes over, the protection won't work unless you specify a +page.server.ts for each file you want to protect.

So instead of the example above, I recommend adding the authorization logic in the +page.server.ts file, see this blog post for a good example.

// src/routes/admin/+page.server.ts
import { redirect } from '@sveltejs/kit';
import { clerkClient } from 'clerk-sveltekit/server';

export const load = ({ locals }) => {
    const { userId } = locals.auth;

    if (!userId) {
        return redirect(307, '/sign-in');
    }

    const user = await clerkClient.users.getUser(userId);

    return {
        user: JSON.parse(JSON.stringify(user))
    };
};

wobsoriano commented 2 months ago

I think right now, I need help finding the issue why it’s not respecting the redirect_url after signing in

@wobsoriano I'll try to help where I can.

Any update on this redirect issue? I am also experiencing the issue.

I will open another PR for this. Still investigating on Clerk side.

wobsoriano commented 1 month ago

Not adding anymore updates to this so we can start reviewing. Thanks! 🚀

wobsoriano commented 1 month ago

@markjaquith We'd love your feedback on our new SDK Development spec!

Spec page: https://clerk.com/docs/references/sdk/overview Sample Discussion: https://github.com/Hawxy/Clerk.Net/discussions/12

andremacola commented 1 month ago

Any update on this PR to merge?

ceifa commented 1 month ago

Looking forward for this PR too! Huge improvement!

LiamKrenn commented 1 month ago

Any updates?

plagesribeiro commented 1 month ago

Looking forward to this merge!

mattjsingleton commented 1 month ago

As am I!

wobsoriano commented 1 month ago

Sorry for the added commits! That's for adding a preview workflow, and now you can test this PR if you want!

npm i https://pkg.pr.new/wobsoriano/clerk-sveltekit@60262e8

pnpm add https://pkg.pr.new/wobsoriano/clerk-sveltekit@60262e8

LekoArts commented 1 month ago

I'll also help with reviewing this PR 👍🏻

For anyone who waits for an update or has shown interest, please try above version in a branch of yours. More testing the better!

mattjsingleton commented 1 month ago

Amazing, I'll hopefully get time time to try this today.

wobsoriano commented 1 month ago

@mattjsingleton the server handler example is in the README but I just updated this PR's description to have it as well

mattjsingleton commented 1 month ago

@mattjsingleton the server handler example is in the README but I just updated this PR's description to have it as well

Yeh thanks, I noticed and deleted my reply straight after aha. Looking good so far, thanks for all your work!

LiamKrenn commented 1 month ago

The experience with this PR has been mostly great - however I had some issues - like mentioned here: #71

And I have had some random 500 internal server error, when using the client data server side - which lead to a "bricked" cookie state, where the user couldn't connect until all cookies were cleared. Unfortunately I can only retrieve the logs tomorrow and provide more information

wobsoriano commented 1 month ago

when using the client data server side

@LiamKrenn do you mean you're accessing the stores server side? It should only be used on client. If you want access to current user, use locals.auth. See the README for more info

LiamKrenn commented 1 month ago

@wobsoriano Sorry, no, I just access the clerk connection to fetch the user and the locals data on the server.

This is my current Implementation - I just wrapped a try around everything for now: https://github.com/SchlexanderOlz/euro-kat/blob/main/sveltekat%2Fsrc%2Fhooks.server.ts

As I said, I'll provide the logs tomorrow

SchlexanderOlz commented 1 month ago

@wobsoriano Sorry, no, I just access the clerk connection to fetch the user and the locals data on the server.

This is my current Implementation - I just wrapped a try around everything for now: https://github.com/SchlexanderOlz/euro-kat/blob/main/sveltekat%2Fsrc%2Fhooks.server.ts

As I said, I'll provide the logs tomorrow

The logs in question @wobsoriano: logs_result.csv

SchlexanderOlz commented 1 month ago

@wobsoriano Previous error was fixed now I am getting this one:

error: Use event.cookies.set(name, value, options) instead of event.setHeaders to set cookies at Object.setHeaders (file:///var/task/.svelte-kit/output/server/index.js:2561:17) at file:///var/task/.svelte-kit/output/server/chunks/hooks.server.js:78:13 at async respond (file:///var/task/.svelte-kit/output/server/index.js:2636:22) at async Server.default (file:///var/task/.svelte-kit/vercel-tmp/index.js:47:3) at async Server.<anonymous> (/opt/rust/nodejs.js:8:4242)

wobsoriano commented 1 month ago

@SchlexanderOlz Oh I forgot SK don't like setting cookies via headers. Updated! Please install with

npm i https://pkg.pr.new/wobsoriano/clerk-sveltekit@60262e8

qtchaos commented 1 month ago

Awesome, I just wanted to update this lib and see that the progress has been going amazing here!

phouverneyuff commented 1 month ago

Can I cache the clerk token? In my test, the browser continues to pull the clerk token every 10 seconds, even though the expiration time has yet to be reached.

ceifa commented 1 month ago

The SignedIn component disables SSR of the subsequential pages/components, because it depends on the auth store. If there is a server-side authentication why it was implemented that way?

wobsoriano commented 1 month ago

@ceifa that is a good question. There's a delay before the auth store gets the value from server in this line.

I think to make the control components (SignedIn, SignedOut, Protect) truly SSR, we'd need a Provider component that accepts a value from the server (app layout). Similar to this one. Then we can pass the stores into context.

mattjsingleton commented 4 weeks ago

Not sure what I’m missing, but in a svelte 5 project I can’t import the client/stores in my layout.ts Any ideas?

wobsoriano commented 3 weeks ago

Not sure what I’m missing, but in a svelte 5 project I can’t import the client/stores in my layout.ts Any ideas?

Would love to know the errors you're getting

LiamKrenn commented 3 weeks ago

Not sure what I’m missing, but in a svelte 5 project I can’t import the client/stores in my layout.ts Any ideas?

Would love to know the errors you're getting

I think Svelte 5 is a bit out of scope for this PR

wobsoriano commented 3 weeks ago

What do you all think of replacing the initializeClerkClient function with an actual provider component?

So in your root layout:

<script lang="ts">
    import { ClerkProvider } from 'clerk-sveltekit';
    import { PUBLIC_CLERK_PUBLISHABLE_KEY } from '$env/static/public';

    // props
</script>

<ClerkProvider {...$$restProps} publishableKey={PUBLIC_CLERK_PUBLISHABLE_KEY}>
    <slot />
</ClerkProvider>

This will allow us to "Actually" have an SSR initial state:

import { buildClerkSSRProps } from 'clerk-sveltekit/server';

// To enable Clerk SSR support, pass the `initialState` to the `ClerkProvider` component.
export const load = ({ locals }) => {
    return {
        ...buildClerkSSRProps(locals.auth)
    };
};

<script lang="ts">
    import { ClerkProvider } from 'clerk-sveltekit';
    import { PUBLIC_CLERK_PUBLISHABLE_KEY } from '$env/static/public';

        export let data: PageData;
    // other props
</script>

<ClerkProvider {...data} {...$$restProps} publishableKey={PUBLIC_CLERK_PUBLISHABLE_KEY}>
    <slot />
</ClerkProvider>

mattjsingleton commented 3 weeks ago

Not sure what I’m missing, but in a svelte 5 project I can’t import the client/stores in my layout.ts Any ideas?

Would love to know the errors you're getting

Tried it in the morning and had no issue, not sure what it was. I'm trying Zed as an editor but can't see how that would've caused it

mattjsingleton commented 3 weeks ago

The "Reset your password" code input form swaps back to the password entry form almost straight away without input or interaction. Tried in Safari and Firefox

To replicate: Enter email on SignIn component, click Continue > Click forgot password > Click Reset your password button

I receive the code from Clerk via email but the form swaps back to the password input. No errors being logged

wobsoriano commented 3 weeks ago

Opened another PR replacing initializeClerkClient hook with a <ClerkProvider> component. This enables better SSR support! See https://github.com/wobsoriano/clerk-sveltekit/pull/2.

I will merge it to this PR this week

Edit: Merged, try it out here:

npm i https://pkg.pr.new/wobsoriano/clerk-sveltekit@3c75a42

wobsoriano commented 2 weeks ago

Hi @markjaquith, I just wanted to check in if you've had a chance to review this PR, or if there's anything I can do to help move things forward. No rush at all 🫡

Again, no pressure at all - just super excited to have Clerk Core 2 with SvelteKit. Thanks for all your work on this project!

markjaquith commented 2 weeks ago

Howdy! I kept seeing activity and have been waiting for it to settle (plus been busy). But you think it's good to go now?

wobsoriano commented 2 weeks ago

Howdy! I kept seeing activity and have been waiting for it to settle (plus been busy). But you think it's good to go now?

Hey! Yeah my friend @LekoArts gave it a review to make sure it passes the Clerk SDK development guide and yes, this is good to go!

ceifa commented 2 weeks ago

@wobsoriano what is the deal with https://github.com/wobsoriano/svelte-clerk ? Liked it because it was built using svelte 5, that integrates better with svelte 5 projects. The idea is to PR that repo to this one in the future? Or it is a standalone fork that you are planning to mantain?

wobsoriano commented 2 weeks ago

@wobsoriano what is the deal with https://github.com/wobsoriano/svelte-clerk ? Liked it because it was built using svelte 5, that integrates better with svelte 5 projects. The idea is to PR that repo to this one in the future? Or it is a standalone fork that you are planning to mantain?

The idea is indeed to PR that repo to this one in the future

ceifa commented 1 week ago

Any special reason for this useClerkContext function? It feels more like react than svelte.

wobsoriano commented 1 week ago

Any special reason for this useClerkContext function? It feels more like react than svelte.

It's a function to get the Clerk context. Without it, the module will break and you will not be able to access Clerk auth state.

https://github.com/markjaquith/clerk-sveltekit/blob/201ac45690cb0f65b12e528af17e0638e32c25af/src/lib/client/context.ts#L34,L43

I didn't mean it to sound react-y. Should we rename it?

Guandor commented 1 week ago

Will this be merged soon?

ceifa commented 1 week ago

I didn't mean it to sound react-y. Should we rename it?

I think the old idea of making the stores individual exports are more svelty. A big issue that contexts have is that they can only be used on component initialization, instead of stores that have a get helper function.

This is the code I'm trying to use with your PR (and gives an error on trying to access the context): https://hastebin.com/share/losukucico.typescript

markjaquith / clerk-sveltekit

Upgrade to Core 2 #60