fix(slot): Change slot to memoize #372
Conversation
SimYunSup
requested review from
MaxLee-dev,
ZeroChoi2781 and
noahchoii
as code owners
🦋 Changeset detectedLatest commit: 24042b2 The changes in this PR will be included in the next version bump. 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 |
|
@SimYunSup is attempting to deploy a commit to the goorm Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @SimYunSup, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the internal mechanism for handling component slots by transitioning from a utility function ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 createSlot을 메모이제이션이 적용된 useSlot 훅으로 변경하여 성능을 개선하려는 좋은 의도를 가지고 있습니다. 하지만 현재 useSlot 훅의 구현과 사용 방식에 몇 가지 중요한 문제가 있어, 의도한 대로 동작하지 않거나 오히려 성능 저하를 일으킬 수 있습니다.
주요 문제는 useSlot 내부의 useMemo 의존성 배열이 불완전하고, useSlot을 호출할 때 인라인 JSX를 전달하여 메모이제이션이 깨지는 것입니다.
자세한 내용은 각 파일에 남긴 리뷰 코멘트를 참고해주세요. 이 문제들을 해결하면 PR의 본래 목적인 성능 최적화를 달성할 수 있을 것입니다.
|
|
||
| const { size } = useBreadcrumbContext(); | ||
| const IconElement = createSlot(children || <SlashOutlineIcon size="100%" />); | ||
| const IconElement = useSlot(children, <SlashOutlineIcon size="100%" />); |
There was a problem hiding this comment.
useSlot에 인라인 JSX를 fallback 인자로 전달하면 useSlot 내부의 useMemo가 매번 다시 실행되어 메모이제이션이 깨집니다. 이는 이 PR의 목적인 성능 최적화에 반하는 동작입니다.
fallback으로 사용되는 <SlashOutlineIcon size="100%" />는 props가 변경되지 않으므로, 컴포넌트 외부에 상수로 선언하여 useSlot에 전달하면 이 문제를 해결할 수 있습니다.
// 예시
const slashIcon = <SlashOutlineIcon size="100%" />;
export const BreadcrumbSeparator = forwardRef<HTMLLIElement, BreadcrumbSeparator.Props>(
(props, ref) => {
// ...
const IconElement = useSlot(children, slashIcon);
// ...
},
);이 패턴은 이 파일의 183번째 줄과 다른 컴포넌트 파일들에서도 동일하게 발견됩니다. 모든 useSlot 사용 부분을 검토하여 인라인 JSX 대신 안정적인 참조를 사용하도록 수정해주세요.
| const dataAttrs = createDataAttributes({ invalid }); | ||
|
|
||
| const ThumbElement = useMemo(() => createSlot(<SwitchThumbPrimitive />), []); | ||
| const ThumbElement = useSlot(<SwitchThumbPrimitive />); |
There was a problem hiding this comment.
기존의 useMemo(() => createSlot(...), []) 코드를 useSlot(<SwitchThumbPrimitive />)으로 변경하면서 성능 저하가 발생했습니다.
useSlot 훅에 인라인 JSX 요소인 <SwitchThumbPrimitive />를 직접 전달하면, SwitchRoot 컴포넌트가 리렌더링될 때마다 새로운 요소 참조가 생성됩니다. 이로 인해 useSlot 내부의 useMemo가 매번 다시 실행되어 ThumbElement 컴포넌트가 계속해서 재생성됩니다. 이는 상태를 잃게 만들고, 이 PR의 목적인 메모이제이션의 이점을 무효화시킵니다.
useSlot의 인자로 전달되는 JSX를 useMemo로 감싸서 안정적인 참조를 유지하도록 수정해야 합니다.
| const ThumbElement = useSlot(<SwitchThumbPrimitive />); | |
| const ThumbElement = useSlot(useMemo(() => <SwitchThumbPrimitive />, [])); |
There was a problem hiding this comment.
2025-12-03.9.57.44.mov
// switch.stories.tsx
<Switch.Root checked={checked1} onCheckedChange={setChecked1} {...args} />The first Switch component in the <Default /> story of switch.stories.tsx does not animate smoothly.
export const SwitchRoot = forwardRef<HTMLButtonElement, SwitchRoot.Props>((props, ref) => {
// ...
const ThumbElement = useSlot(<SwitchThumbPrimitive />);
const children = childrenProp || <ThumbElement />;In the above code, if childrenProp is absent, it always renders ThumbElement (SwitchThumbPrimitive). Inside useSlot, it memoizes the Slot component by depending on _children (SwitchThumbPrimitive). However, inline JSX always creates a new object. Therefore, when childrenProp is absent, a new component is created each time, making it appear as if the animation isn't applied.
There was a problem hiding this comment.
Thanks for testing! I changed code to memoize rendered element.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Close because #389 is integrated PR. |
3 participants
Related Issues
Fixes #371
Description of Changes
Change createSlot to
useSlothook and wrapuseMemoChecklist
Before submitting the PR, please make sure you have checked all of the following items.