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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(elements): Verification form not submitting after returning from ChooseStrategy without a selection #3425

Merged
merged 8 commits into from
May 23, 2024

Conversation

tmilewski
Copy link
Member

@tmilewski tmilewski commented May 22, 2024

Description

Problem: The form wasn't submitting after the NAVIGATE.PREVIOUS event from the ChooseStrategy state.

Reason: The top-level state was in the correct place, but it wasn't being propagated to First Factor so the SUBMIT event was sent to ChooseStrategy and not Pending as it should've been.

Solution: Propagate the event to the First Factor child machine.


Adds a NeverRetriable state to Pending so as not to run the countdown, if not necessary. Ultimately, making debugging a bit easier.


Fixes SDK-1765
Fixes SDK-1766

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

  • 馃悰 Bug fix
  • 馃専 New feature
  • 馃敤 Breaking change
  • 馃摉 Refactoring / dependency upgrade / documentation
  • other:

Copy link

changeset-bot bot commented May 22, 2024

馃 Changeset detected

Latest commit: e2c1ad2

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

This PR includes changesets to release 1 package
Name Type
@clerk/elements 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
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/backend 1.2.1-snapshot.veb34b56
@clerk/chrome-extension 1.0.14-snapshot.veb34b56
@clerk/clerk-js 5.5.1-snapshot.veb34b56
@clerk/elements 0.5.0-snapshot.veb34b56
@clerk/clerk-expo 1.1.6-snapshot.veb34b56
@clerk/express 0.0.10-snapshot.veb34b56
@clerk/fastify 1.0.11-snapshot.veb34b56
gatsby-plugin-clerk 5.0.0-beta.45
@clerk/nextjs 5.1.1-snapshot.veb34b56
@clerk/clerk-react 5.2.1-snapshot.veb34b56
@clerk/remix 4.0.12-snapshot.veb34b56
@clerk/clerk-sdk-node 5.0.9-snapshot.veb34b56
@clerk/shared 2.2.1-snapshot.veb34b56
@clerk/testing 1.1.4-snapshot.veb34b56

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

npm i @clerk/backend@1.2.1-snapshot.veb34b56 --save-exact

@clerk/chrome-extension

npm i @clerk/chrome-extension@1.0.14-snapshot.veb34b56 --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@5.5.1-snapshot.veb34b56 --save-exact

@clerk/elements

npm i @clerk/elements@0.5.0-snapshot.veb34b56 --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@1.1.6-snapshot.veb34b56 --save-exact

@clerk/express

npm i @clerk/express@0.0.10-snapshot.veb34b56 --save-exact

@clerk/fastify

npm i @clerk/fastify@1.0.11-snapshot.veb34b56 --save-exact

gatsby-plugin-clerk

npm i gatsby-plugin-clerk@5.0.0-beta.45 --save-exact

@clerk/nextjs

npm i @clerk/nextjs@5.1.1-snapshot.veb34b56 --save-exact

@clerk/clerk-react

npm i @clerk/clerk-react@5.2.1-snapshot.veb34b56 --save-exact

@clerk/remix

npm i @clerk/remix@4.0.12-snapshot.veb34b56 --save-exact

@clerk/clerk-sdk-node

npm i @clerk/clerk-sdk-node@5.0.9-snapshot.veb34b56 --save-exact

@clerk/shared

npm i @clerk/shared@2.2.1-snapshot.veb34b56 --save-exact

@clerk/testing

npm i @clerk/testing@1.1.4-snapshot.veb34b56 --save-exact

'NAVIGATE.PREVIOUS': {
description: 'Go to Idle, and also tell firstFactor to go to Pending',
target: 'Idle',
actions: sendTo(({ self }) => self.getSnapshot().children['firstFactor']!, { type: 'NAVIGATE.PREVIOUS' }),
Copy link
Member

Choose a reason for hiding this comment

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

What would happen if children['firstFactor'] is undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't the best way to handle this in this scenario. I used it as I was working on something else where the children approach made sense. I've updated it.

That said, technically, it wouldn't ever be undefined in this scenario as firstFactor is invoked in this state's parent.

@@ -145,6 +145,7 @@ const SignInVerificationMachine = setup({
},
guards: {
isResendable: ({ context }) => context.resendable || context.resendableAfter === 0,
isNeverResendable: ({ context }) => context.currentFactor?.strategy === 'password',
Copy link
Member

@LekoArts LekoArts May 23, 2024

Choose a reason for hiding this comment

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

Aren't there more strategies that are never resendable?
For example: passkey, google_one_tap, saml

Copy link
Member Author

Choose a reason for hiding this comment

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

There are, though I wasn't looking to address each of them in this PR. Especially, as passkey and google_one_tap haven't been integrated yet. While retaining the other flow isn't ideal, it doesn't cause any issues either.

@tmilewski tmilewski force-pushed the elements/fix-choose-strategy-go-back branch from 8543b0c to b87a7d3 Compare May 23, 2024 15:26
Base automatically changed from elements/inspector to main May 23, 2024 19:57
@tmilewski
Copy link
Member Author

Pushing through as integration tests are having issues unrelated to this PR.

@tmilewski tmilewski merged commit c7ff401 into main May 23, 2024
10 of 11 checks passed
@tmilewski tmilewski deleted the elements/fix-choose-strategy-go-back branch May 23, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants