feat(ui): start adding use authenticate calls to the modal #649
Conversation
moldy530
requested review from
rthomare,
dancoombs,
mokok123 and
dphilipson
as code owners
moldy530
force-pushed
the
moldy/useAuth-inModal
branch
2 times, most recently
from
2e6f5ba
to
9144aae
Compare
| ); | ||
| }, | ||
| [header, sections] | ||
| ); |
There was a problem hiding this comment.
We may want to write this so that AuthModal is the same instance even if header or sections change. As written, if either of those change, then AuthModal will be a different function instance which will trigger a remount. I can especially see this happening with sections, where someone might end up writing their code in a way that constructs new array instances to pass in on every render.
One way we could write this:
const latestHeader = useLatestRef(header);
const latestSections = useLatestRef(sections);
// Ensure same instance on every render.
const AuthModal = useRef(
({ className }: ModalProps) => {
// ...
<AuthCard header={latestHeader.current} sections={latestSections.current} />
// ...
}
).current;where useLatestRef does what it sounds like:
function useLatestRef<T>(value: T): MutableRefObject<T> {
const ref = useRef(value);
ref.current = value;
return ref;
}There was a problem hiding this comment.
There may be some edge cases where that proposed solution won't work, since changing header or sections wouldn't trigger a re-render in AuthModal. Normally this would be fine since if you're calling useAuthModal chances are you're about re-render some JSX that has the AuthModal in it, but I could imagine some kinds of application structure where you don't use AuthModal immediately and it not re-rendering could be a problem. If you think that's worth worrying about, we probably need some mechanism to send the new values to the AuthModal to trigger a re-render.
There was a problem hiding this comment.
Oh hmmm I actually ran into this not with section and headers since I was expecting these to be mostly static. Even in the tech spec one proposal is to bake this uiConfig into the createConfig function which is supposed to be a static reference.
I ran into this with the input component. If you have the input focused and the input validator generates an error then the whole input component is re-rendered because the error prop is different. This causes the input to lose focus. For Input I need to come back to handle that better, probably with the way you've proposed.
For AuthModal or similar components in other libs, my expectation would be that changes to those props would result in a re-render.
One thing I was thinking about is actually NOT returning the <AuthModal /> component to the dev at all. Instead, I think the hook should inject the modal into the dom (likely using a portal). Then all of these edge cases with re-renders are confined within the hooks implementation and we don't have to make assumptions about external consumers. I think this is how most UI libraries do it normally. What are your thoughts on taking this approach instead?
There was a problem hiding this comment.
@dphilipson what do you think about: #655? I've not addressed the re-renders that can happen when ui-config changes, but now at least the rendering of the dialog is done in the same component that receives the props which should match expectations better?
moldy530
force-pushed
the
moldy/useAuth-inModal
branch
from
53f3ddf
to
a2da04d
Compare
Pull Request Checklist
yarn test)sitefolder, and guidelines for updating/adding docs can be found in the contribution guide)feat!: breaking change)yarn lint:check) and fix any issues? (yarn lint:write)PR-Codex overview
This PR focuses on adding new dependencies, updating UI components, and refactoring file structure in the Alchemy project.
Detailed summary
@tanstack/react-formand@tanstack/zod-form-adapterAlchemyLogo,ChevronRight,MailIcon