-
Notifications
You must be signed in to change notification settings - Fork 10
fix(core): Change useMutationObserver to useMutationObserverRef #332
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
Open
SimYunSup
wants to merge
9
commits into
goorm-dev:main
Choose a base branch
from
SimYunSup:claude/analyze-library-review-011CUsgFBJCiumAaXkMa9yd3
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
b0e9bb9
refactor(core): Replace useMutationObserver with callback ref pattern
claude f1a63fa
refactor(useMutationObserverRef): Remove outdated reference link in d…
SimYunSup 3eb728b
refactor(core): Update useMutationObserver hook to utilize only ref
SimYunSup 4baa089
fix(core): Ensure observerRef is cleared only if it matches the curre…
SimYunSup 4e09a65
refactor(test): Update testRef initialization for consistency in useM…
SimYunSup 17d3a64
style(core): Simplify export statement for useMutationObserverRef hook
SimYunSup 4a6d218
fix(core): Fix TypeScript type errors in useMutationObserverRef tests
claude 353969a
Merge branch 'main' into claude/analyze-library-review-011CUsgFBJCium…
SimYunSup 7259abb
chore: Format via prettier
SimYunSup File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@vapor-ui/core': patch | ||
| --- | ||
|
|
||
| Change useMutationObserver hook to use only ref |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
201 changes: 201 additions & 0 deletions
201
packages/core/src/hooks/use-mutation-observer-ref.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,201 @@ | ||
| import { render, waitFor } from '@testing-library/react'; | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { useMutationObserverRef } from './use-mutation-observer-ref'; | ||
|
|
||
| describe('useMutationObserverRef', () => { | ||
| it('should observe mutations on the attached element', async () => { | ||
| const callback = vi.fn(); | ||
| let testRef = null as HTMLDivElement | null; | ||
|
|
||
| function TestComponent() { | ||
| const ref = useMutationObserverRef<HTMLDivElement>({ | ||
| callback, | ||
| options: { attributes: true, attributeFilter: ['data-test'] }, | ||
| }); | ||
|
|
||
| return ( | ||
| <div | ||
| ref={(node) => { | ||
| testRef = node; | ||
| ref(node); | ||
| }} | ||
| data-test="initial" | ||
| > | ||
| Test Content | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| render(<TestComponent />); | ||
|
|
||
| // Wait for the component to mount and observer to be set up | ||
| await waitFor(() => expect(testRef).not.toBeNull()); | ||
|
|
||
| // Trigger a mutation | ||
| testRef!.setAttribute('data-test', 'changed'); | ||
|
|
||
| // Wait for the callback to be called | ||
| await waitFor(() => { | ||
| expect(callback).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| expect(callback.mock.calls[0][0]).toHaveLength(1); | ||
| expect(callback.mock.calls[0][0][0].type).toBe('attributes'); | ||
| expect(callback.mock.calls[0][0][0].attributeName).toBe('data-test'); | ||
| }); | ||
|
|
||
| it('should disconnect observer when ref is set to null', async () => { | ||
| const callback = vi.fn(); | ||
|
|
||
| function TestComponent({ show }: { show: boolean }) { | ||
| const ref = useMutationObserverRef<HTMLDivElement>({ | ||
| callback, | ||
| options: { attributes: true }, | ||
| }); | ||
|
|
||
| if (!show) return null; | ||
|
|
||
| return ( | ||
| <div ref={ref} data-test="value"> | ||
| Test Content | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const { rerender } = render(<TestComponent show={true} />); | ||
|
|
||
| // Unmount the component | ||
| rerender(<TestComponent show={false} />); | ||
|
|
||
| // The observer should be disconnected, so no further mutations should be observed | ||
| await waitFor(() => { | ||
| expect(callback).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| it('should update to the latest callback on each render', async () => { | ||
| const firstCallback = vi.fn(); | ||
| const secondCallback = vi.fn(); | ||
| let testRef = null as HTMLDivElement | null; | ||
|
|
||
| function TestComponent({ callback }: { callback: (mutations: MutationRecord[]) => void }) { | ||
| const ref = useMutationObserverRef<HTMLDivElement>({ | ||
| callback, | ||
| options: { attributes: true, attributeFilter: ['data-test'] }, | ||
| }); | ||
|
|
||
| return ( | ||
| <div | ||
| ref={(node) => { | ||
| testRef = node; | ||
| ref(node); | ||
| }} | ||
| data-test="initial" | ||
| > | ||
| Test Content | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const { rerender } = render(<TestComponent callback={firstCallback} />); | ||
|
|
||
| await waitFor(() => expect(testRef).not.toBeNull()); | ||
|
|
||
| // Trigger first mutation | ||
| testRef!.setAttribute('data-test', 'changed1'); | ||
|
|
||
| await waitFor(() => { | ||
| expect(firstCallback).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // Clear the first callback and switch to second | ||
| firstCallback.mockClear(); | ||
| rerender(<TestComponent callback={secondCallback} />); | ||
|
|
||
| // Trigger second mutation | ||
| testRef!.setAttribute('data-test', 'changed2'); | ||
|
|
||
| await waitFor(() => { | ||
| expect(secondCallback).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // First callback should not have been called again | ||
| expect(firstCallback).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should observe child list mutations when configured', async () => { | ||
| const callback = vi.fn(); | ||
| let testRef = null as HTMLDivElement | null; | ||
|
|
||
| function TestComponent() { | ||
| const ref = useMutationObserverRef<HTMLDivElement>({ | ||
| callback, | ||
| options: { childList: true }, | ||
| }); | ||
|
|
||
| return ( | ||
| <div | ||
| ref={(node) => { | ||
| testRef = node; | ||
| ref(node); | ||
| }} | ||
| > | ||
| <span>Initial Child</span> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| render(<TestComponent />); | ||
|
|
||
| await waitFor(() => expect(testRef).not.toBeNull()); | ||
|
|
||
| // Add a new child | ||
| const newChild = document.createElement('span'); | ||
| newChild.textContent = 'New Child'; | ||
| testRef!.appendChild(newChild); | ||
|
|
||
| await waitFor(() => { | ||
| expect(callback).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| expect(callback.mock.calls[0][0][0].type).toBe('childList'); | ||
| }); | ||
|
|
||
| it('should return cleanup function that disconnects observer (React 19+ support)', async () => { | ||
| const callback = vi.fn(); | ||
| let cleanupFn: (() => void) | undefined; | ||
|
|
||
| function TestComponent() { | ||
| const ref = useMutationObserverRef<HTMLDivElement>({ | ||
| callback, | ||
| options: { attributes: true }, | ||
| }); | ||
|
|
||
| return ( | ||
| <div | ||
| ref={(node) => { | ||
| const cleanup = ref(node); | ||
| // Store cleanup function if returned (React 19+) | ||
| if (typeof cleanup === 'function') { | ||
| cleanupFn = cleanup; | ||
| } | ||
| }} | ||
| data-test="initial" | ||
| > | ||
| Test Content | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const { unmount } = render(<TestComponent />); | ||
|
|
||
| // If cleanup function was returned, it should work | ||
| if (cleanupFn) { | ||
| cleanupFn(); | ||
| expect(callback).not.toHaveBeenCalled(); | ||
| } | ||
|
|
||
| unmount(); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import { useCallback, useRef } from 'react'; | ||
|
|
||
| interface Params { | ||
| callback: (mutations: MutationRecord[]) => void; | ||
| options?: MutationObserverInit; | ||
| } | ||
|
|
||
| /** | ||
| * A hook that returns a callback ref which sets up a MutationObserver | ||
| * when attached to a DOM element. | ||
| * | ||
| * This implementation uses callback refs instead of useEffect to avoid | ||
| * issues with stale dependencies and unnecessary observer recreation. | ||
| * It stores the callback and options in refs to maintain stable references | ||
| * across renders, ensuring compatibility with React 17+. | ||
| * | ||
| * For React 19+, the callback ref returns a cleanup function that will | ||
| * automatically disconnect the observer when the ref changes or unmounts. | ||
| * | ||
| * @see https://tkdodo.eu/blog/ref-callbacks-react-19-and-the-compiler | ||
| * | ||
| * @example | ||
| * const ref = useMutationObserverRef({ | ||
| * callback: (mutations) => console.log(mutations), | ||
| * options: { attributes: true } | ||
| * }); | ||
| * | ||
| * return <div ref={ref}>Content</div>; | ||
| */ | ||
| export const useMutationObserverRef = <T extends HTMLElement>({ callback, options }: Params) => { | ||
| const observerRef = useRef<MutationObserver | null>(null); | ||
| const callbackRef = useRef(callback); | ||
| const optionsRef = useRef(options); | ||
|
|
||
| // Keep refs up to date with latest values | ||
| callbackRef.current = callback; | ||
| optionsRef.current = options; | ||
|
|
||
| const refCallback = useCallback((node: T | null) => { | ||
| // Cleanup previous observer if it exists | ||
| if (observerRef.current) { | ||
| observerRef.current.disconnect(); | ||
| observerRef.current = null; | ||
| } | ||
|
|
||
| // Set up new observer if node is present | ||
| if (node) { | ||
| // Use the refs to get the latest callback and options | ||
| const observer = new MutationObserver((mutations) => { | ||
| callbackRef.current(mutations); | ||
| }); | ||
| observer.observe(node, optionsRef.current); | ||
| observerRef.current = observer; | ||
|
|
||
| // Return cleanup function for React 19+ | ||
| // For React 17-18, this return value is ignored | ||
| return () => { | ||
| observer.disconnect(); | ||
| if (observerRef.current === observer) { | ||
| observerRef.current = null; | ||
| } | ||
| }; | ||
| } | ||
| }, []); // Empty dependency array - stable across all renders | ||
|
|
||
| return refCallback; | ||
| }; |
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should return cleanup function that disconnects observer (React 19+ support)테스트 케이스가 정리 함수의 동작을 올바르게 검증하지 못하고 있습니다.현재 테스트는
cleanupFn()을 호출한 직후callback이 호출되지 않았는지만 확인합니다. 하지만 이 시점에는 어떠한 DOM 변경도 발생하지 않았기 때문에callback은 원래 호출되지 않는 것이 정상입니다.정리 함수가 observer를 제대로
disconnect하는지 확인하려면,cleanupFn()호출 이후에 DOM 변경을 발생시키고, 그 결과로callback이 호출되지 않는 것을 확인해야 합니다.아래와 같이 테스트를 수정하여 정리 함수의 동작을 더 정확하게 검증하는 것을 제안합니다.