-
Notifications
You must be signed in to change notification settings - Fork 408
fix(clerk-js,backend): Do not depend on _clerk_synced to terminate satellite sync flow #4516
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
Conversation
| } | ||
|
|
||
| private shouldUseSuffixed(): boolean { | ||
| public usesSuffixedCookies(): boolean { |
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.
It's the same code - git diff messes with formatting here.
I dropped the suffixedCookies prop that was getting initialized in the ctor in favor of a simple function to simplify things a little.
ad3da85 to
ffa1009
Compare
🦋 Changeset detectedLatest commit: 92a8a1f The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
a42af24 to
883b8f2
Compare
| const isRequestEligibleForMultiDomainSync = | ||
| authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document'; | ||
|
|
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 actual change, correct ?
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.
Correct. I will check the clerk_go side shortly but we can push these changes independently
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 clerk-js change is also important as we couldn't remove the param before for nextjs apps.
| removeClerkQueryParam(CLERK_SYNCED); | ||
| removeClerkQueryParam('__clerk_handshake'); | ||
| removeClerkQueryParam('__clerk_help'); |
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.
Does it make sense to batch these ? To avoid calling replaceState multiple times ? Can this affect routers from host frameworks ?
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 don't think it makes a difference as we're calling replace here but I can definitely test
|
!snapshot |
|
Hey @nikosdouvlis - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/astro@1.4.9-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/backend@1.16.1-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/chrome-extension@1.3.32-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/clerk-js@5.33.0-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/elements@0.19.1-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/clerk-expo@2.3.1-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/expo-passkeys@0.0.3-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/express@1.3.11-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/fastify@2.0.13-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/localizations@3.6.1-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/nextjs@6.3.1-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/clerk-react@5.15.3-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/remix@4.2.49-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/clerk-sdk-node@5.0.62-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/shared@2.12.1-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/tanstack-start@0.4.25-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/testing@1.3.23-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/themes@2.1.43-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/types@4.32.0-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/ui@0.1.18-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact
npm i @clerk/vue@0.0.2-snapshot.v883b8f2a0ce72a656f1f61a1c1fc8aa933e51212 --save-exact |
…tellite sync flow
883b8f2 to
349dd3b
Compare
| // @nikos: we're looking into dropping this param completely | ||
| // in the meantime, we're removing it here to keep the URL clean | ||
| removeClerkQueryParam(CLERK_SUFFIXED_COOKIES); |
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 should never make it to the client, I'm pretty sure it being added to the redirect is a bug in FAPI that needs to be fixed.
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.
That's exactly the case, I'll get to the bottom of it tomorrow
…tellite sync flow
Description
WIP - description to be added soon
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change