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

refactor: attempt to improve performance #1057

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

moldy530
Copy link
Collaborator

@moldy530 moldy530 commented Oct 12, 2024

  • refactor: make some performance optimizations
  • refactor(wip): add selectors for theme items

Pull Request Checklist


PR-Codex overview

This PR focuses on refactoring the UI configuration management within the account-kit library, transitioning from direct config usage to a centralized store approach. It updates various components to utilize the new store, enhancing modularity and maintainability.

Detailed summary

  • Removed direct config usage in favor of useConfigStore.
  • Updated components to access UI state via the new store.
  • Introduced useUiConfig for contextual UI configuration.
  • Adjusted package dependencies for zustand.
  • Enhanced theme handling in multiple components.
  • Refactored modal and authentication handling to utilize the new store structure.

The following files were skipped due to too many changes: examples/ui-demo/src/state/store.tsx, account-kit/react/src/components/auth/sections/EmailAuth.tsx, account-kit/react/src/icons/oauth.tsx, yarn.lock

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

vercel bot commented Oct 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
aa-sdk-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 12, 2024 5:38am
aa-sdk-ui-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 12, 2024 5:38am

<div
className="transition-[flex-basis] duration-200 ease-out overflow-y-hidden radius-2"
style={{
flexBasis: height ? `${height}px` : undefined,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

animating flex basis should be more performant than the height

@@ -2,7 +2,8 @@ import { useUiConfig } from "../../../../hooks/useUiConfig.js";
import { ls } from "../../../../strings.js";

export const HelpText = () => {
const { supportUrl } = useUiConfig();
const { supportUrl } = useUiConfig(({ supportUrl }) => ({ supportUrl }));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

most of these changes for the useConfig are done so that components subscribe just to the state they care about. This results in WAY fewer renders

setAuthStep({ type: "initial", error });
},
});
export const EmailAuth = memo(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this component is pretty expensive to render, so I'm just memoizing it because in demo app it is always present. This saves on a bunch of renders

@@ -8,7 +9,7 @@ type Props = Extract<AuthType, { type: "social" }>;

// Not used externally
// eslint-disable-next-line jsdoc/require-jsdoc
export const OAuth = ({ ...config }: Props) => {
export const OAuth = memo(({ ...config }: Props) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this component is also super expensive to render and I haven't figure out why. This also saves a number of renders too

@@ -17,31 +16,31 @@ type ButtonProps = (
"variant" | "ref"
>;

export const Button = forwardRef<HTMLButtonElement, ButtonProps>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

forward ref was unnecessary here, so removing it to try and optimize a bit of memory. had no impact really

@@ -23,7 +23,7 @@ export const Navigation = ({
disabled={!showBack}
className={
showBack
? "text-fg-secondary w-[32px] h-[32px] flex items-center justify-center hover:bg-btn-secondary rounded-md transition-all ease-out duration-200"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from what I could tell, these animations weren't doing anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored the above uiConfig store to be more like the one in the demo app. This gives us the ability to subscribe to specific pieces of the ui config in the components

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

some optimizations of the svg components to make them a bit lighterweight

Comment on lines +18 to +23
const localAlchemyConfig = {
...alchemyConfig,
ui: props.initialConfig
? convertDemoConfigToUiConfig(props.initialConfig)
: alchemyConfig.ui,
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ensures that the initial config is inline with the cached config if there is one

Custom
</p>
<div
className={cn("flex gap-x-3", auth.showOAuth ? "" : "hidden")}
Copy link
Collaborator Author

@moldy530 moldy530 Oct 12, 2024

Choose a reason for hiding this comment

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

this makes rendering the authentication config sidebar a bit faster

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

more SVG optimizations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and again

return [config.ui.theme];
}

export function deepEqual(a: any, b: any): boolean {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because the demo app's config store isn't super deep or complex, this ensures that fewer renders take place when nested state is updated. the cost of the deep equal is sub 1ms after doing some testing

@moldy530
Copy link
Collaborator Author

Even after all this though... I'm still seeing dropped frames whenever the height of the card changes. I tested in a different app and I'm not seeing frames dropped, or if they are they're not really perceptible.

These changes result in like 4ish fewer dropped frames. The worst offender is adding or removing the OAuth buttons. Updating that middle section of the card causes the most dropped frames.

Adding or removing the external wallets button used to be as slow as the middle section. It's now noticeably smoother.

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.

1 participant