feat(@aws-amplify/amplify-ui-components): authenticator slotted toast

[Luke-Davies]

Issue #, if available: #6479

Description of changes:
Following discussions on PR #7129 and issue #6479, this PR offers a slotted approach to toast customisation in the amplify-authenticator component.

This PR has been updated. The example is the same however.

Rather than just wrapping the amplify-toast in a slot however, this PR introduces a new component: amplify-auth-alert.

Introducing this component has the following benefits:

• Removes the handleToastEvent logic from amplify-authenticator, therefore reducing the logic/responsibility in this component.
• amplify-auth-alert can be used outside/without the amplify-authenticator if desired.
• Provides a clean example of an alert component in amplify-auth-alert for anyone looking to roll their own alert component.

React (ts) example:

import React, { useState, useEffect } from 'react';
import { Hub, HubCallback } from '@aws-amplify/core';
import {
  AmplifyAuthenticator,
} from '@aws-amplify/ui-react';
import {
 UI_AUTH_CHANNEL, TOAST_AUTH_ERROR_EVENT
} from '@aws-amplify/ui-components';

const MyAlert: React.FC<{ slot: string }> = ({ slot }) => {
  const [alertMessage, setAlertMessage] = useState('');

  const handleToastErrors: HubCallback = ({ payload }) => {
    if (payload.event === TOAST_AUTH_ERROR_EVENT && payload.message) {
      setAlertMessage(payload.message);
    }
  };

  useEffect(() => {
    Hub.listen(UI_AUTH_CHANNEL, handleToastErrors);
    return () => Hub.remove(UI_AUTH_CHANNEL, handleToastErrors);
  });

  return (<span slot={slot}>{alertMessage}</span>);
};

const MyAuth: React.FC = ({ children }) => {
  return (
    <>
      <AmplifyAuthenticator>
        <MyAlert slot="notification" />
        // ...........
      </AmplifyAuthenticator>
    </>
  );
}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[Luke-Davies]

The logic behind the component name amplify-auth-alert is that is reflects the fact that it only alerts on TOAST_AUTH_ERROR_EVENT.

Note that the slot name in amplify-authenticator is just alert however.

Open to better suggestions!

[codecov]

Codecov Report

Merging #7601 (ed25321) into main (7a15725) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #7601   +/-   ##
=======================================
  Coverage   74.13%   74.13%           
=======================================
  Files         215      215           
  Lines       13450    13450           
  Branches     2641     2641           
=======================================
  Hits         9971     9971           
  Misses       3280     3280           
  Partials      199      199           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a15725...ed25321. Read the comment docs.

[wlee221]

Hi! Thanks a lot for diving deep into this. Apologies this got long time to respond also. I'm a bit against creating a new component though:

• amplify-auth-alert will be used by amplify-authenticator only. Other auth components are nested under amplify-authenticator and thus covered by amplify-authenticator's error handling logic.
• This spawns new lifecycle methods that we need to manage. I see that you implemented getAlertSlotWithComponent, which I really appreciate! But this shouldn't be necessary if we just wrapping amplify-toast in a slot.
• End user code will still look the same to override / hide toasts.

So I think we want to keep it simple in this case, but please feel free to disagree! @ericclemmons, what do you think?

[Luke-Davies]

Thanks for taking a look @wlee221. You don't need to apologise for taking time to get to it!
I disagree with some of those points but I might be misunderstanding a few things:

---

amplify-auth-alert will be used by amplify-authenticator only. Other auth components are nested under amplify-authenticator and thus covered by amplify-authenticator's error handling logic.

amplify-auth-alert is usable outside of amplify-authenticator. We'd all expect most developers to be using the authenticator, but not all of them will. That said, I mainly thought about this from the perspective that introducing amplify-auth-alert improves amplify-authenticator by reducing the scope of its responsibility (more on that below), rather than the value it brings as a usable, stand-alone component.

I would view it in the same way as amplify-sign-in and amplify-sign-up for example (but much simpler). Although they can be used without it, those components are really intended for use in the authenticator, but you wouldn't move everything they do directly into the authenticator.

Could you help me identify the error handling logic in amplify-authenticator that amplify-auth-alert is escaping?

---

This spawns new lifecycle methods that we need to manage. I see that you implemented getAlertSlotWithComponent, which I really appreciate! But this shouldn't be necessary if we just wrapping amplify-toast in a slot.

For life cycle methods, are you referring to stencil's lifecycle methods? If so, could you help me understand what you mean when you say "we need to manage" these? It is likely something I haven't understood about web components or stencil.

If you're referring simply to the hub listeners that amplify-auth-alert creates, I believe this actually prevents unnecessary hub listeners being created when overriding the alert slot of amplify-authenticator. If we just wrapped the toast in a slot and kept the hub listeners (and state) in amplify-authenticator, whenever anyone overrides the toast slot, the authenticator would still be creating hub listeners that would no longer provide any benefit (since they simply manage state that is only used by the default toast component). By moving the hub listeners (and state) to amplify-auth-alert, overriding the alert slot of amplify-authenticator results in no redundant hub listeners.

---

End user code will still look the same to override / hide toasts.

I think this is actually a positive for this solution since it demonstrates that no additional complexity has been added in that regard.

---

I'm not sure how I ended up writing so much text sorry!

But the tl;dr would be that amplify-auth-alert provides value by reducing the responsibility of amplify-authenticator. Being a reusable component in it's own right is more of a bonus.

[wlee221]

Hi @Luke-Davies, thanks for the detailed response!

---

I would view it in the same way as amplify-sign-in and amplify-sign-up for example [...] those components are really intended for use in the authenticator, but you wouldn't move everything they do directly into the authenticator.

I see, so this is more about separating the logic from amplify-authenticator. My concern was that amplify-auth-alert wouldn't be used much used outside amplify-authenticator, but your comment addresses my concern.

---

For life cycle methods, are you referring to stencil's lifecycle methods?

Yes. #7522 discusses this, but in tl;dr, inserting web components into a slot is very unpredictable. But

amplify-js/packages/amplify-ui-components/src/components/amplify-authenticator/amplify-authenticator.tsx

Line 178 in 7790e8f

return <slot name={slotName}>{slotIsEmpty && alertComponent}</slot>;

would catch those errors, so I'm good.

---

If we just wrapped the toast in a slot [...] the authenticator would still be creating hub listeners that would no longer provide any benefit

This is a good catch!

---

I think this is actually a positive for this solution since it demonstrates that no additional complexity has been added in that regard.

My point was that having amplify-auth-alert doesn't enable more customization options than just using a slot (could've clarified that better). But to your comment, I see the value in separation of concerns. And we get to remove unnecessary Hub listeners as a bonus :)

---

I'm not sure how I ended up writing so much text sorry!

Don't worry, I (unnecessarily) write 5 paragraph essays to my PRs and issues too 😛

---

Overall, I'm convinced and for this change! I think 49 lines long component is worth this refactor. I'll add some reviews later.

Edit: We want to keep the solution simple in this case; see comment below.

[wlee221]

LGTM, with some nits 👍 Also, can you add slot documentation at the top of amplify-authenticator? Something like

amplify-js/packages/amplify-ui-components/src/components/amplify-authenticator/amplify-authenticator.tsx

Line 27 in ac0ef4b

* @slot sign-in - Content placed inside of the sign in workflow for when a user wants to sign into their account

See comments below

[wlee221]

Hi @Luke-Davies, I have gave this more thought and also synced internally on this pull request. I apologize about taking back my previous comment, but we want to move away from creating a new component for this case.

Our goal for issue #6479 was to be able to hide toast, which a simple slot addition in amplify-authenticator achieves in two additional lines. Adding a new component for this does not enable more use case in this aspect; If customers want to add their own implementation, they would still have to write their own component to customize the text or visuals. This is unlike amplify-sign-in or other subcomponents that encapsulate integration logic with the Amplify JS library and provides multiple customization option via props. amplify-auth-alert by itself isn't much reusable, and amplify-toast provides sufficient abstraction for general notification purpose or to use for custom alert component.

You brought up a good point about code-splitting and removing unnecessary Hub event. For code-splitting, I stand by my previous point that amplify-toast provides enough abstraction that amplify-authenticator does not need additional splitting at the cost of additional layer and bundle size increase. For Hub events, you are absolutely right that Hub events would run even when slot is taken. But I think this is an acceptable trade-off, because this only happens when customer is using the slot functionality and even then we're not doing anything computationally intensive there.

That being said, thanks a lot for diving deep and putting a lot of thought into this! Sorry again for taking back what I said -- It's really that we want to prioritize simpler solution for this use case. Let's go back to #7129 and continue there!

[Luke-Davies]

Sorry for only just getting back to this @wlee221. Thanks for the detailed explanation, that makes a lot of sense. Would it be better to adapt this PR to remove the new component? I can just drop the commits from my fork and reimplement it. That seems to make more sense based on the names of the two PRs.

I'll hold off until I hear back.

[wlee221]

@Luke-Davies, it would be totally fine to use this PR.

[Luke-Davies]

@wlee221 see brand-new branch history.

Let me know if the name "alert" is ok for the slot. Or do we want "toast"?

I'll update the PR description next although I think the example is exactly the same.

[wlee221]

Now the hardest part... naming! I'm thinking about notification, as suggested from the previous thread. This has the neutral connotation and it conveys what the slot is for well, which I like over others.

[wlee221]

Can we have this.toastMessage && check here in line 190 so the slot isn't always visible?

[Luke-Davies]

I thought about this but came to the conclusion that it would restrict the slot to only render based on how authenticator's default slot works.

A rushed example to illustrate what I mean:

<AmplifyAuthenticator>
    <div slot="alert">
        {authErrMsg && ( // authErrMsg is the same as authenticators toastMessage
            <MyFancyAlert msg={authErrMsg} handleClose={() => setAuthErrMsg('')} />
        )}
        {showInactiveSignOutMsg && (
            <MyFancyAlert msg={"You were signed out due to inactivity."} handleClose={() => setShowInactiveSignOutMsg(false)} />
        )}
    </div>
</AmplifyAuthenticator>

That example wouldn't work if we move this.toastMessage && down a line.

The "inactive sign out" bits could be moved outside the authenticator, but then so could the authErrMsg bit and that would leave me wondering how much benefit there is to the slot approach vs the props approach.

That said, most usages will look like the example I put in the description so I might be over thinking this.

[Luke-Davies]

I guess one counter argument to what I've said would be that, without moving this.toastMessage &&, there's nothing to stop people abusing this slot for non-notification purposes.

I'm really starting to wonder if the prop approach may be the better option here. It seems like with the slot approach we have a choice between either a slot that is really open - so more customisation at the risk of being mis-used - or a slot that is pretty restrictive - i.e. can only really do the same as what the default component does functionality-wise.

Either way I think I've talked myself into the change you've suggested.

[wlee221]

You bring up a good point -- I really like your inactivity example. I'm also in agreement with the prop approach. Instead of having a limited slot / too open of a slot, we can just have hideToast and user can do whatever.

I think this is a better developer experience 1) for customers just trying to hide toast, and 2) customers trying to customize toast, because they have more flexibility on their notification implementation. Custom implementation can be placed anywhere and can be reused outside AmplifyAuthenticator for general notification purpose:

const MyAuth: React.FC = ({ children }) => {
  return (
    <div>
      <MyAlert />
      <AmplifyAuthenticator>
        // ...........
      </AmplifyAuthenticator>
    </div>
  );
}

[Luke-Davies]

Agreed. How do we proceed now? I could write an explanation on #7129 outlining why props may be a better choice? Or should I leave this with yourself for now @wlee221?

If the consensus is still slots, it would just be a question of whether it is a limited or open slot.

If there is agreement on props then I believe #7129 is good to go, although we could make a minor improvement which I'll discuss there.

[wlee221]

Let's go ahead and use props since we're in agreement. We can move over to #7129, and thanks for your responsiveness over this!

[Luke-Davies]

I've just noticed I missed the @slot comment again. Will get this added and rename the slot.

[Luke-Davies]

Closed in favour of #7129

[github-actions]

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.