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

Enable useAuthenticator usage outside <Authenticator /> #1168

Merged
merged 26 commits into from
Jan 26, 2022

Conversation

wlee221
Copy link
Contributor

@wlee221 wlee221 commented Jan 19, 2022

Issue #, if available: #637, #254, #664, #640.

Description of changes: This PR enables useAuthenticator hook to be used outside Authenticator. Also adds /useAuthenticator example and e2e tests that'll supplement docs PR #1075.

Use Case

Main use case is custom routing (#1058), which is not possible today because Authenticator assumes SPA usage and requires Authenticated content to be nested inside Authenticator. #1130 puts it well:

The Quick Start instructions are great... if you want the entire app locked down. Beyond that... it's not immediately clear to me (and likely others, too) how I could exclude certain pages from Authenticator. (eg, Home, About, etc.).

Problem

Internally, we used useAuthenticator and Authenticator.Provider to share auth contexts. To use useAuthenticator outside Authenticator, we falsely went around this issue by wrapping <Authenticator.Provider /> around their app:

(1) <Authenticator.Provider> 
(2)   <App>
(3)     <Authenticator> 
(4)       <Authenticator.Provider>
(5)         <Authenticator.SignIn />
(6)        ...

Problem here was that two providers create two different xstate machines. useAuthenticator from <App /> (2) returned false data because it is referencing the wrong provider (1) instead of (4).

Wasted re-renders

Another React nuance we overlooked is that useAuthenticator changes on every xstate transitions, including form input events!

This is not only computationally expensive but also can trigger repeated mounting of <Authenticator /> because MyApp (1) encompasses it. This opens up crucial bugs like form losing its values on re-render. xstate docs explains better than I can:

React context can be a tricky tool to work with - if you pass in values which change too often, it can result in re-renders all the way down the tree.

Solutions

Sync Provider Context Value

Now Authenticator.Provider detects whether there's another Provider above it. If so, it'll just pass that parent value. This resolves multiple xstate machine instantiations.

Serve static value from Authenticator.Provider

Now Authenticator.Provider only maintains a static reference to authMachine service to prevent re-renders, as suggested from xstate docs. useAuthenticator instead listens to state changes and provides respective auth context.

Split Slot logic to internal hook

We've been using useAuthenticator to share custom components for Authenticator. This is only needed internally, is static, and is not needed externally. This has been split to useCustomComponents hook to focus Authenticator.Provider on just authMachine contexts. Splitting up complex context value is encouraged to reduce wasted re-renders: facebook/react#15156.

Add optimization mechanism to useAuthenticator

Even with static served Authenticator.Provider, useAuthenticator still changes frequently on every state change. This PR adds useAuthenticatorRoute and useAuthenticatorUser that only triggers re-render whenever route or user change respectively. This provides developers control over when they want auth updates.

Under the hood, this is done by selector optional parameter to base useAuthenticator. selector is a function that accepts current facade values and returns desired value(s) that should trigger re-render.

This uses xstate's useSelector, which provides just what we need:

This hook will only cause a rerender if the selected value changes, ...

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@changeset-bot
Copy link

changeset-bot bot commented Jan 19, 2022

🦋 Changeset detected

Latest commit: 480d76f

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

This PR includes changesets to release 1 package
Name Type
@aws-amplify/ui-react Minor

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

@wlee221 wlee221 temporarily deployed to ci January 19, 2022 08:36 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 08:36 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 08:36 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 09:50 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 09:50 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 09:50 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 09:50 Inactive
@lgtm-com
Copy link

lgtm-com bot commented Jan 19, 2022

This pull request introduces 1 alert when merging af70d1e into e0fcf36 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jan 19, 2022

This pull request introduces 1 alert when merging 1e9b3d7 into e0fcf36 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@wlee221 wlee221 temporarily deployed to ci January 19, 2022 13:11 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 13:11 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 13:11 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 13:11 Inactive
@lgtm-com
Copy link

lgtm-com bot commented Jan 19, 2022

This pull request introduces 1 alert when merging c361bb9 into e0fcf36 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@wlee221 wlee221 temporarily deployed to ci January 19, 2022 13:25 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 13:25 Inactive
@wlee221 wlee221 temporarily deployed to ci January 19, 2022 13:25 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 02:39 Inactive
wlee221 added a commit that referenced this pull request Jan 26, 2022
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 04:29 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 04:29 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 04:29 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 04:29 Inactive
Copy link
Contributor

@ErikCH ErikCH left a comment

Choose a reason for hiding this comment

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

Looks good!

@wlee221 wlee221 enabled auto-merge (squash) January 26, 2022 18:58
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 19:02 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 19:02 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 19:02 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 19:02 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 19:03 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 19:03 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 19:03 Inactive
@wlee221 wlee221 temporarily deployed to ci January 26, 2022 19:03 Inactive
@wlee221 wlee221 merged commit b32dd86 into main Jan 26, 2022
@wlee221 wlee221 deleted the headless/sync-providers branch January 26, 2022 19:14
@github-actions github-actions bot mentioned this pull request Jan 26, 2022
wlee221 added a commit that referenced this pull request Jan 27, 2022
* Initial draft for React

* Remove full-headless related content

* Restore custom slot example

* First attempt at useAuthenticator example

* Initial draft

* First attempt at useAuthenticator example

* Allow children to be null

* Remove initial state

* Provider now only stores static ref to service

* match new exports

* add useAuthenticator test

* remove unused variable

* Create weak-stingrays-raise.md

* Create aero-stingrays-raise.md

* Fix typo

* add more comments

* Add comment on selector choice

* useAuthenticator only returns selected values

* Update changesets

* update exports

* Fix types

* update exports list

* Update .changeset/aero-stingrays-raise.md

* Update .changeset/weak-stingrays-raise.md

* Update react documentation to match #1168

* revert non pr related chagnes

* Add angular example

* Trigger tests

* run angular tests

* Added Vue examples and documentation

* Vue tests will run

* selector returns an array of dependencies

* Update test name

* Update react documentation to match latest #1168

* Add angular docs

* Correct full-api link

* Remove unused variable

* Add instructions for injecting AuthenticatorService

* Remove unused export

* Remove lingering comment

* Change 'conditional-rendering' to `current-route'

* Send INIT event from I18n demo

* Fix typos

* wordiness

Co-authored-by: Erik Hanchett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants