-
Notifications
You must be signed in to change notification settings - Fork 297
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
Convert useAuthenticator hook & services to public APIs #532
Conversation
8e61ae5
to
2d7d0b3
Compare
const useAuthenticatorValue = ({ | ||
components: customComponents, |
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.
Will this now run on import, instead of when the Authenticator component is initialized?
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.
Good question – this is still created at run-time when someone provides:
<AuthenticatorProvider {...components, services} />
It's not invoked as a side-effect here.
42a18ec
to
05f410c
Compare
I'm going to get tests passing on this refactor and then do a PR documenting & supporting this use-case. That way, that PR can be done to support multiple frameworks. |
@ErikCH One of the changes I need to make for Vue is so that there are both amplify-ui/packages/vue/src/components/confirm-sign-up.vue Lines 122 to 133 in 40eef17
|
@@ -3,6 +3,7 @@ | |||
<base-wrapper v-bind="$attrs"> | |||
<base-form | |||
data-amplify-authenticator-confirmsignin | |||
@input="onInput" |
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 looks fine
|
||
export type ProviderProps = AuthenticatorMachineOptions & { | ||
components?: typeof defaultComponents; | ||
services?: undefined; |
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.
What is this going to be?
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.
Something like:
services: {
signUp({ username, password }) {
return Auth.signUp({ username, password }, myCustomOptions);
},
validateSignUp(data) {
// run custom validation
}
}
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.
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 Vue changes looks good. Looks like this will take some further updates on the Vue side to start using this new updateForm
and submitForm
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.
Looks good, no blockers!
@@ -38,6 +38,9 @@ jobs: | |||
- name: Install packages | |||
run: yarn --frozen-lockfile | |||
|
|||
- name: Lint packages | |||
run: yarn workspace ${{ matrix.package }} lint |
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.
Then should this be done after yarn [framework]-example build
on line 102?
|
||
const { challengeName, remoteError } = actorState.context as SignInContext; |
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.
NON_BLOCKING: How do you envision exposing validationErrors
? I thought validate*
as fire and forget functions from customer-end that doesn't return back to the customer:
const services = { validateSignUp };
return withAuthenticator(app, { services});
|
||
const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
const { name, value } = event.target; | ||
updateForm({ name, value }); |
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 love this!
]); | ||
|
||
const isPending = | ||
state.hasTag('pending') || getActorState(state)?.hasTag('pending'); |
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.
✨
const { confirmation_code: code } = event.data; | ||
const { confirmation_code: code } = context.formValues; |
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.
Right, but I'd be careful about this though because we were inconsistent about sending CHANGE events.
I see that you've bridged that gap on React/Vue -- I'll follow it up from Angular as well.
amplify-ui/packages/react/src/components/Authenticator/ConfirmSignIn/ConfirmSignIn.tsx
Lines 41 to 44 in 1ae6bac
<Form | |
data-amplify-authenticator-confirmsignin="" | |
method="post" | |
onSubmit={(event) => { |
@wlee221 To answer this:
It would be available via: const { error, validationErrors /* or formErrors? */ } = useAuthenticator(); The service is still fire-and-forget, but on |
Description of changes:
This PR paves the way for custom components & public APIs using React's
useAuthenticator
hook: #552<AuthenticatorProvider {...initialState, components, loginMechanisms, services } />
useAuthenticator
no longer returns[state, send]
and instead returns only public APIs.state.matches(...)
withroute
(signIn
|verifyUser
| ...). This doesn't include pending states, which will be indicated via anisPending
facade._send
and_state
but/** @deprecated */
so we can track & remove these private values.isPending
facade. We want to avoid any usage ofstate.matches(...)
in components!signIn
andsignUp
facades totransitionToSignIn
ortoSignIn
orgotoSignIn
or something for @wlee221Future Changes
Add example for how to customize Sign Up #552
Fix Angular linting errors #549
Replace all usage of
state.matches
in componentsI'm not a fan of
submitForm
andupdateForm
vs. explicitly sayingupdateSignUpForm
orsubmitSignUpForm
.Update the docs example to actually work.
Replace all usage of
send(...)
in componentsBecause
loginMechanisms
is pretty much reverted (https://docs.amplify.aws/cli/migration/cli-auth-signup-changes/),usernameAliases = [email | phone_number]
is consistent with:socialProviders = ['amazon', 'facebook', 'google']
would differentiate the two.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.