Skip to content
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

fix(clerk-js,types): Reuse existing sign-up if available #4720

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kostaspt
Copy link
Member

@kostaspt kostaspt commented Dec 5, 2024

Description

Currently, when a user lands on the sign-up component with a ticket, it sends a request to the Clerk API and starts a new sign-up attempt. If the process requires a password, it will start another new sign-up attempt on submit, losing the original context.

The issue occurs mainly when the server responds with a 422 error because of e.g. a pwned password. If the user then switches to an OAuth strategy, the initial sign-up attempt should be reused with the same context instead of starting over and potentially losing data.

This PR fixes the issue by adding an upsert method to the SignUp resource. This method reuses the existing sign-up attempt ID if one already exists. While this change could have been made directly in place, adding this new method seems to me like a cleaner approach. No strong opinions here though to revert to that.

Checklist

  • pnpm test runs as expected.
  • pnpm 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

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Copy link

changeset-bot bot commented Dec 5, 2024

🦋 Changeset detected

Latest commit: 6c06ff6

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

This PR includes changesets to release 23 packages
Name Type
@clerk/clerk-js Minor
@clerk/types Minor
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/astro Patch
@clerk/backend Patch
@clerk/elements Patch
@clerk/expo-passkeys Patch
@clerk/express Patch
@clerk/fastify Patch
@clerk/localizations Patch
@clerk/nextjs Patch
@clerk/nuxt Patch
@clerk/react-router Patch
@clerk/clerk-react Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
@clerk/shared Patch
@clerk/tanstack-start Patch
@clerk/testing Patch
@clerk/themes Patch
@clerk/ui Patch
@clerk/vue 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

Copy link

vercel bot commented Dec 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
clerk-js-sandbox ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 10, 2024 9:50am

'@clerk/types': minor
---

Introduced an `upsert` method to the `SignUp` resource, which reuses the existing sign-up attempt ID if it exists. This was an obvious oversight in the ticket flow, so `SignUpStart` has been updated to use this instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a changelog entry that is read by our users, so we don't include context that is internal to us. I would go with a more straight-forward approach like:

Suggested change
Introduced an `upsert` method to the `SignUp` resource, which reuses the existing sign-up attempt ID if it exists. This was an obvious oversight in the ticket flow, so `SignUpStart` has been updated to use this instead.
Fix ticket flow on `<SignUp />` component, where in some rare cases the initial ticket/context is lost.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Also, you can mention that an upsert function is introduced on the SignUp resource, because it can be used by custom flow implementations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reconsidering this and I would go for this changeset with minor changes and this text:

Suggested change
Introduced an `upsert` method to the `SignUp` resource, which reuses the existing sign-up attempt ID if it exists. This was an obvious oversight in the ticket flow, so `SignUpStart` has been updated to use this instead.
Introduced an `upsert` method to the `SignUp` resource, which reuses the existing sign-up attempt ID if it exists.

And then I would add another changeset with patch changes and this text:

Suggested change
Introduced an `upsert` method to the `SignUp` resource, which reuses the existing sign-up attempt ID if it exists. This was an obvious oversight in the ticket flow, so `SignUpStart` has been updated to use this instead.
Fix ticket flow on `<SignUp />` component, where in some rare cases the initial ticket/context is lost.

So we will have 2 different entries on our changelog!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that makes sense @anagstef! Thanks for the suggestions – updated.

@clerk-cookie
Copy link
Collaborator

Hey @kostaspt - the snapshot version command generated the following package versions:

Package Version
@clerk/astro 2.0.1-snapshot.v20241210095051
@clerk/backend 1.20.3-snapshot.v20241210095051
@clerk/chrome-extension 2.0.9-snapshot.v20241210095051
@clerk/clerk-js 5.41.0-snapshot.v20241210095051
@clerk/elements 0.21.4-snapshot.v20241210095051
@clerk/clerk-expo 2.4.5-snapshot.v20241210095051
@clerk/expo-passkeys 0.0.17-snapshot.v20241210095051
@clerk/express 1.3.26-snapshot.v20241210095051
@clerk/fastify 2.0.28-snapshot.v20241210095051
@clerk/localizations 3.8.2-snapshot.v20241210095051
@clerk/nextjs 6.8.3-snapshot.v20241210095051
@clerk/nuxt 0.0.13-snapshot.v20241210095051
@clerk/clerk-react 5.19.3-snapshot.v20241210095051
@clerk/react-router 0.0.2-snapshot.v20241210095051
@clerk/remix 4.3.6-snapshot.v20241210095051
@clerk/clerk-sdk-node 5.0.77-snapshot.v20241210095051
@clerk/shared 2.19.4-snapshot.v20241210095051
@clerk/tanstack-start 0.7.0-snapshot.v20241210095051
@clerk/testing 1.3.38-snapshot.v20241210095051
@clerk/themes 2.1.55-snapshot.v20241210095051
@clerk/types 4.40.0-snapshot.v20241210095051
@clerk/ui 0.2.4-snapshot.v20241210095051
@clerk/vue 0.0.17-snapshot.v20241210095051

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/astro

npm i @clerk/[email protected] --save-exact

@clerk/backend

npm i @clerk/[email protected] --save-exact

@clerk/chrome-extension

npm i @clerk/[email protected] --save-exact

@clerk/clerk-js

npm i @clerk/[email protected] --save-exact

@clerk/elements

npm i @clerk/[email protected] --save-exact

@clerk/clerk-expo

npm i @clerk/[email protected] --save-exact

@clerk/expo-passkeys

npm i @clerk/[email protected] --save-exact

@clerk/express

npm i @clerk/[email protected] --save-exact

@clerk/fastify

npm i @clerk/[email protected] --save-exact

@clerk/localizations

npm i @clerk/[email protected] --save-exact

@clerk/nextjs

npm i @clerk/[email protected] --save-exact

@clerk/nuxt

npm i @clerk/[email protected] --save-exact

@clerk/clerk-react

npm i @clerk/[email protected] --save-exact

@clerk/react-router

npm i @clerk/[email protected] --save-exact

@clerk/remix

npm i @clerk/[email protected] --save-exact

@clerk/clerk-sdk-node

npm i @clerk/[email protected] --save-exact

@clerk/shared

npm i @clerk/[email protected] --save-exact

@clerk/tanstack-start

npm i @clerk/[email protected] --save-exact

@clerk/testing

npm i @clerk/[email protected] --save-exact

@clerk/themes

npm i @clerk/[email protected] --save-exact

@clerk/types

npm i @clerk/[email protected] --save-exact

@clerk/ui

npm i @clerk/[email protected] --save-exact

@clerk/vue

npm i @clerk/[email protected] --save-exact

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants