-
Notifications
You must be signed in to change notification settings - Fork 23
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
Upgrade to Core 2 #60
base: main
Are you sure you want to change the base?
Conversation
src/app.d.ts
Outdated
'clerk-sveltekit:user': CustomEvent<UserResource> | ||
} |
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.
Don't think we need this anymore?
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.
I didn't need it in my (minimal) testing
Hi any status update on this one ? Maybe we can help ? |
src/lib/server/route-matcher.ts
Outdated
* Path patterns and regular expressions are supported, for example: `['/foo', '/bar(.*)'] or `[/^\/foo\/.*$/]` | ||
* For more information, see: https://clerk.com/docs | ||
*/ | ||
export const createRouteMatcher = (routes: RouteMatcherParam) => { |
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.
This is the same helper function exported by the Nextjs SDK that helps matching routes to be protected
src/lib/server/withClerkHandler.ts
Outdated
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.
The middleware is overhauled to use the authenticateRequest
function to match existing Clerk server SDKs.
It authenticates every request, and determines if the user is signed in, signed out or if a handshake should happen.
src/lib/client/SignIn.svelte
Outdated
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.
We should let Clerk handle the redirects with the new middleware
@7Kronos any help would be much appreciated! Today I implemented new server hooks. Mark is busy at work atm btw. |
@wobsoriano what help would you need to get this draft into a mergeable reality? |
@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 |
@wobsoriano I'll try to help where I can. |
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. |
Hey @markjaquith! Will try and fix the tests today and marked it as ready for review. |
fix clerk store type cleanup chore: use built-in goto function for navigations chore!: bump @clerk/backend to 1.2.3 update readme clenaup use derived store for resources feat: add Protect component update error message update readme chore: add store export chore: add JSDoc to stores chore: update JSDoc examples feat: introduce better request authentication in backend chore: remove deprecated options chore: use built-in store for loaded and loading components chore: remove test results chore: run prettier chore: update deprecated props in unstyled components chore: bump @clerk/backend to 1.3.0 chore: bump @clerk/shared to 2.3.1 playground update type updates fix env vars type fixes remove unused export clean up fix type exports update readme update readme update readme update readme update peer deps tmp updates remove internal components update unstyled sign out button props remove logs improve SSR state move initial state setting in client hook move initial state setting in client hook cleanup type fix cleanup cleanup remove @sveltejs/kit peer dependency Fix path problem Co-authored-by: Dustin Delatore <[email protected]> remove matcher test: Simplify tests chore: Update env example and remove unused function
Hi @markjaquith, marked as ready for review! Just FYI
|
Will this be merged soon? |
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 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 |
Ah I see your point. The main reason with the context/provide of stores is to make Clerk control components ( You can still do |
We would specifically love to be able to have SSR support and get the token on server-side, so would be great to avoid going back to client-side only :) |
await expect(serverSecret).toContainText(SERVER_SECRET) | ||
|
||
// Can see the user button. | ||
const userButton = page.getByTestId('user-button') |
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.
Is this still being expected? Just downloaded the code and tried to run the tests:
Error: Timed out 5000ms waiting for expect(locator).toBeVisible()
Locator: getByTestId('user-button')
Expected: visible
Received: <element(s) not found>
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.
I think it can be related to the organization switcher throwing an error because organizations are not enabled on my account...
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.
@wobsoriano on my code I would like to have stores that rely on the clerk user, working server-side and client-side, with your PR I don't see how it would be possible, for the same reason of my previous comment (sorry for being annoying on this subject). I tried to create some code on your PR to understand if it could be a thing or not: wobsoriano#3, what do you think? |
Bumping this one, in case anyone wants to use this branch in their project: npm install https://pkg.pr.new/wobsoriano/clerk-sveltekit@ca15d4e |
@@ -162,19 +170,19 @@ | |||
"author": "Mark Jaquith <[email protected]>", | |||
"license": "MIT", | |||
"peerDependencies": { | |||
"svelte": "^4.0.0||^5.0.0", | |||
"@sveltejs/kit": "^1.25.1||^2.0.2" | |||
"svelte": "^4.0.0 || ^5.0.0" |
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.
Now that Svete v5 is stable, it should be safe to remove v3 as peer dependency 🫡
Hi, sorry for the annoying ping again @markjaquith. My recent commit was just updating README and updating Clerk dependencies. Feel free to review when you have the time 😃 |
This is the week! I'm blocking time for this tomorrow. This should be a major version, and then the next major version can be a Svelte 5 release. So people will be able to use any combination of Clerk components and Svelte versions. |
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.
This is incredible work.
Do you think a specific upgrade guide is needed, or is "follow the setup guide, and obviously replace the old stuff with this new stuff" sufficient?
```ts | ||
import { withClerkHandler } from 'clerk-sveltekit/server' | ||
|
||
export const handle = withClerkHandler() |
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.
I think we should still show how to use sequence()
to combine multiple handlers, otherwise the installation steps might be unclear to someone who already has an existing handler/handlers.
@@ -0,0 +1,11 @@ | |||
import { constants } from '@clerk/backend/internal' | |||
import { env as privateEnv } from '$env/dynamic/private' |
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.
Is there a reason for these being imported with "dynamic" instead of "static"?
could do this, but maybe there are better options:
import * as privateEnv from '$env/static/private';
import * as publicEnv from '$env/static/public';
using "dynamic" breaks "prerendering" on some of my pages.
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.
I might have a solution for this
~~Yeah I think an upgrade guide would be helpful! I'm thinking if we should just add another markdown file for this 🤔 ~~ and thank you for your time reviewing this! Update: @markjaquith I think we can safely skip adding a migration guide as Core 2 is there for a year already and Clerk docs is pure Core 2 🤔 |
@wobsoriano I just stumbled upon this PR, thank you so much for your work. It makes life much easier, especially being able to cross-reference the docs and follow them more closely makes using this library much more comfortable. I tried to look for a way of switching organizations (on the client, as server-side, this seems unsupported by clerk themselves), but I couldn't find the correct function to use. I checked almost all the stores exported by |
Update: |
@wobsoriano Sorry if pinging you again is something you dislike, please feel free to mention it and it won't happen again. After using [this](npm install https://pkg.pr.new/wobsoriano/clerk-sveltekit@ca15d4e) version of the PR, I am now "production ready", encountering only a few bugs, one of which I can't wrap my head around: For my project, I have both verification codes via email and Google OAuth enabled. It looks like when redirecting from clerk.mydomain.com after clerk has initiated a handshake ( The full flow of my app:
|
@markjaquith Are we able to merge this PR? What else is needed? I'd like to upgrade to Svelte 5, and this is blocking us. |
Hi @markjaquith 👋 First, I wanted to say a huge thank you for creating and maintaining this package! I wanted to let you know that I've created svelte-clerk, a Svelte 5-focused implementation (using runes and snippets). This was developed in response to numerous requests we receive at Clerk from developers seeking an official Svelte SDK. I'll be actively maintaining it until we release an official Clerk Svelte SDK. Your project paved the way for this one, and we've acknowledged your work in svelte-clerk's README. Thank you again for your contributions! Please feel free to share any feedback or suggestions you might have. 🙌 |
@wobsoriano That sounds great. I haven't been able to give this the attention it deserves over the last year, and was considering reaching out to you to ask if Clerk would consider supporting Svelte in-house, and this sounds like a path to that. |
We appreciate your efforts @markjaquith! Thank you |
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 insidehook.client.ts
, this introduces a<ClerkProvider>
component to be used in the root layout and accepts the same props as the<ClerkProvider>
React component :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:For server,
authenticateRequest
is implemented, and theAuth
object is injected tolocals.auth
:In
hooks.server.ts
:This is another breaking change since we're removing the
protectedRoutes
option and let userland handle it.Fixes
initializeClerkClient
should be awaited #46<Protect />
component)protectedPaths
not protecting routes with hyphens #67ClerkLoaded
results in type error #68@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 :)