-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(dnd): update useDescription to prevent unnecessary re-renders #9738
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
reidbarber
wants to merge
6
commits into
main
Choose a base branch
from
dnd-perf-useDynamicDescription
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.
+316
−27
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
db44f62
perf(dnd): add useDynamicDescription to prevent unnecessary re-renders
reidbarber f0f8bce
don't create nodes for initial empty descriptions
reidbarber 7b59821
combine behavior into useDescription
reidbarber cb150bd
Merge remote-tracking branch 'origin/main' into dnd-perf-useDynamicDe…
reidbarber ebc40ce
typescript
reidbarber 4e95ffd
Merge branch 'main' into dnd-perf-useDynamicDescription
reidbarber 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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,43 +12,101 @@ | |
|
|
||
| import {AriaLabelingProps} from '@react-types/shared'; | ||
| import {useLayoutEffect} from './useLayoutEffect'; | ||
| import {useState} from 'react'; | ||
| import {useRef, useState} from 'react'; | ||
|
|
||
| let descriptionId = 0; | ||
| const descriptionNodes = new Map<string, {refCount: number, element: Element}>(); | ||
|
|
||
| export function useDescription(description?: string): AriaLabelingProps { | ||
| interface DescriptionNode { | ||
| refCount: number, | ||
| element: HTMLElement | ||
| } | ||
|
|
||
| interface DescriptionSubscription { | ||
| key: string, | ||
| node: DescriptionNode, | ||
| nodes: Map<string, DescriptionNode> | ||
| } | ||
|
|
||
| const descriptionNodes = new Map<string, DescriptionNode>(); | ||
| const dynamicDescriptionNodes = new Map<string, DescriptionNode>(); | ||
|
|
||
| function createDescriptionNode(id: string, description: string): HTMLElement { | ||
| let node = document.createElement('div'); | ||
| node.id = id; | ||
| node.style.display = 'none'; | ||
| node.textContent = description; | ||
| document.body.appendChild(node); | ||
| return node; | ||
| } | ||
|
|
||
| function getOrCreateDescriptionNode(nodes: Map<string, DescriptionNode>, descriptionKey: string, description: string) { | ||
| let desc = nodes.get(descriptionKey); | ||
| if (!desc) { | ||
| let id = `react-aria-description-${descriptionId++}`; | ||
| let node = createDescriptionNode(id, description); | ||
| desc = {refCount: 0, element: node}; | ||
| nodes.set(descriptionKey, desc); | ||
| } | ||
|
|
||
| return desc; | ||
| } | ||
|
|
||
| function cleanupDescriptionSubscription(subscription: DescriptionSubscription, nodeRef: {current: HTMLElement | null}) { | ||
| if (--subscription.node.refCount === 0) { | ||
| subscription.node.element.remove(); | ||
| subscription.nodes.delete(subscription.key); | ||
| } | ||
|
|
||
| if (nodeRef.current === subscription.node.element) { | ||
| nodeRef.current = null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Provides an `aria-describedby` reference to a shared hidden description node. | ||
| * By default, descriptions are shared by exact text. If `descriptionKey` is provided, | ||
| * a stable node is shared by key and its text content updates in place as the | ||
| * description changes. | ||
| */ | ||
| export function useDescription(description?: string, descriptionKey?: string): AriaLabelingProps { | ||
| let [id, setId] = useState<string | undefined>(); | ||
| let subscriptionRef = useRef<DescriptionSubscription | null>(null); | ||
| let nodeRef = useRef<HTMLElement | null>(null); | ||
| let isDynamic = descriptionKey != null; | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!description) { | ||
| return; | ||
| let subscription = subscriptionRef.current; | ||
| let key = descriptionKey ?? description; | ||
| let nodes = isDynamic ? dynamicDescriptionNodes : descriptionNodes; | ||
| if (subscription && (subscription.key !== key || subscription.nodes !== nodes)) { | ||
| cleanupDescriptionSubscription(subscription, nodeRef); | ||
| subscriptionRef.current = null; | ||
| subscription = null; | ||
| } | ||
|
|
||
| if (!subscription && description && key) { | ||
| let node = getOrCreateDescriptionNode(nodes, key, isDynamic ? '' : description); | ||
| node.refCount++; | ||
| subscription = {key, node, nodes}; | ||
| subscriptionRef.current = subscription; | ||
| nodeRef.current = node.element; | ||
| setId(node.element.id); | ||
| } | ||
|
|
||
| let desc = descriptionNodes.get(description); | ||
| if (!desc) { | ||
| let id = `react-aria-description-${descriptionId++}`; | ||
| setId(id); | ||
|
|
||
| let node = document.createElement('div'); | ||
| node.id = id; | ||
| node.style.display = 'none'; | ||
| node.textContent = description; | ||
| document.body.appendChild(node); | ||
| desc = {refCount: 0, element: node}; | ||
| descriptionNodes.set(description, desc); | ||
| } else { | ||
| setId(desc.element.id); | ||
| if (isDynamic && description && nodeRef.current) { | ||
| nodeRef.current.textContent = description; | ||
| } | ||
| }, [description, descriptionKey, isDynamic]); | ||
|
|
||
| desc.refCount++; | ||
| useLayoutEffect(() => { | ||
| return () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you move this up to be in the first useLayoutEffect, then you can de-duplicate the refCount removal, always handle it in the cleanup of that effect |
||
| if (desc && --desc.refCount === 0) { | ||
| desc.element.remove(); | ||
| descriptionNodes.delete(description); | ||
| let subscription = subscriptionRef.current; | ||
| if (subscription) { | ||
| cleanupDescriptionSubscription(subscription, nodeRef); | ||
| subscriptionRef.current = null; | ||
| } | ||
| }; | ||
| }, [description]); | ||
| }, []); | ||
|
|
||
| return { | ||
| 'aria-describedby': description ? id : undefined | ||
|
|
||
145 changes: 145 additions & 0 deletions
145
packages/@react-aria/utils/test/useDescription.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,145 @@ | ||
| /* | ||
| * Copyright 2025 Adobe. All rights reserved. | ||
| * This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. You may obtain a copy | ||
| * of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under | ||
| * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS | ||
| * OF ANY KIND, either express or implied. See the License for the specific language | ||
| * governing permissions and limitations under the License. | ||
| */ | ||
|
|
||
| import {actHook as act, renderHook} from '@react-spectrum/test-utils-internal'; | ||
| import {useDescription} from '../src/useDescription'; | ||
|
|
||
| describe('useDescription', () => { | ||
| it('should return an id if description is provided', () => { | ||
| let {result} = renderHook(() => useDescription('Test description')); | ||
| expect(result.current['aria-describedby']).toMatch(/^react-aria-description-\d+$/); | ||
| }); | ||
|
|
||
| it('should return undefined if no description is provided', () => { | ||
| let {result} = renderHook(() => useDescription()); | ||
| expect(result.current['aria-describedby']).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should reuse the same id for the same description', () => { | ||
| let {result: result1} = renderHook(() => useDescription('Test description')); | ||
| let {result: result2} = renderHook(() => useDescription('Test description')); | ||
| expect(result1.current['aria-describedby']).toBe(result2.current['aria-describedby']); | ||
| }); | ||
|
|
||
| it('should create a new id for a new description', () => { | ||
| let {result: result1} = renderHook(() => useDescription('Test description 1')); | ||
| let {result: result2} = renderHook(() => useDescription('Test description 2')); | ||
| expect(result1.current['aria-describedby']).not.toBe(result2.current['aria-describedby']); | ||
| }); | ||
|
|
||
| it('should clean up description node on unmount', () => { | ||
| let {result, unmount} = renderHook(() => useDescription('Test description')); | ||
| let id = result.current['aria-describedby']; | ||
| expect(document.getElementById(id!)).not.toBeNull(); | ||
| unmount(); | ||
| expect(document.getElementById(id!)).toBeNull(); | ||
| }); | ||
|
|
||
| it('should not clean up if other components are using the same description', () => { | ||
| let {result: result1, unmount: unmount1} = renderHook(() => useDescription('Test description')); | ||
| let {unmount: unmount2} = renderHook(() => useDescription('Test description')); | ||
| let id = result1.current['aria-describedby']; | ||
| expect(document.getElementById(id!)).not.toBeNull(); | ||
| unmount1(); | ||
| expect(document.getElementById(id!)).not.toBeNull(); | ||
| unmount2(); | ||
| expect(document.getElementById(id!)).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('useDescription with a description key', () => { | ||
| it('should return an id if description is provided', () => { | ||
| let {result} = renderHook(() => useDescription('Test description', 'dynamic-1')); | ||
| expect(result.current['aria-describedby']).toMatch(/^react-aria-description-\d+$/); | ||
| }); | ||
|
|
||
| it('should return undefined if no description is provided', () => { | ||
| let {result} = renderHook(() => useDescription(undefined, 'dynamic-2')); | ||
| expect(result.current['aria-describedby']).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should reuse the same id for the same description key', () => { | ||
| let {result: result1} = renderHook(() => useDescription('Test description', 'shared-key')); | ||
| let {result: result2} = renderHook(() => useDescription('Test description', 'shared-key')); | ||
| expect(result1.current['aria-describedby']).toBe(result2.current['aria-describedby']); | ||
| }); | ||
|
|
||
| it('should create a new id for a different description key', () => { | ||
| let {result: result1} = renderHook(() => useDescription('Test description', 'dynamic-3')); | ||
| let {result: result2} = renderHook(() => useDescription('Test description', 'dynamic-4')); | ||
| expect(result1.current['aria-describedby']).not.toBe(result2.current['aria-describedby']); | ||
| }); | ||
|
|
||
| it('should keep the same id and update text content when description changes for the same key', () => { | ||
| let {result, rerender} = renderHook( | ||
| ({description}: {description?: string}) => useDescription(description, 'dynamic-5'), | ||
| {initialProps: {description: 'Test description 1'}} | ||
| ); | ||
|
|
||
| let id = result.current['aria-describedby']; | ||
| let node = document.getElementById(id!); | ||
| expect(node?.textContent).toBe('Test description 1'); | ||
|
|
||
| act(() => { | ||
| rerender({description: 'Test description 2'}); | ||
| }); | ||
|
|
||
| expect(result.current['aria-describedby']).toBe(id); | ||
| expect(document.getElementById(id!)).toBe(node); | ||
| expect(node?.textContent).toBe('Test description 2'); | ||
| }); | ||
|
|
||
| it('should keep the same id for the lifetime of the component', () => { | ||
| let {result, rerender} = renderHook( | ||
| ({description}: {description?: string}) => useDescription(description, 'dynamic-6'), | ||
| {initialProps: {description: 'Test description'}} | ||
| ); | ||
|
|
||
| let id = result.current['aria-describedby']; | ||
| let node = document.getElementById(id!); | ||
|
|
||
| act(() => { | ||
| rerender({description: ''}); | ||
| }); | ||
|
|
||
| expect(result.current['aria-describedby']).toBeUndefined(); | ||
| expect(document.getElementById(id!)).toBe(node); | ||
| expect(node?.textContent).toBe('Test description'); | ||
|
|
||
| act(() => { | ||
| rerender({description: 'Updated description'}); | ||
| }); | ||
|
|
||
| expect(result.current['aria-describedby']).toBe(id); | ||
| expect(document.getElementById(id!)).toBe(node); | ||
| expect(node?.textContent).toBe('Updated description'); | ||
| }); | ||
|
|
||
| it('should not clean up if other components are using the same description key', () => { | ||
| let {result: result1, unmount: unmount1} = renderHook(() => useDescription('Test description', 'shared-cleanup')); | ||
| let {unmount: unmount2} = renderHook(() => useDescription('Test description', 'shared-cleanup')); | ||
| let id = result1.current['aria-describedby']; | ||
| expect(document.getElementById(id!)).not.toBeNull(); | ||
| unmount1(); | ||
| expect(document.getElementById(id!)).not.toBeNull(); | ||
| unmount2(); | ||
| expect(document.getElementById(id!)).toBeNull(); | ||
| }); | ||
|
|
||
| it('should clean up description node on unmount', () => { | ||
| let {result, unmount} = renderHook(() => useDescription('Test description', 'dynamic-7')); | ||
| let id = result.current['aria-describedby']; | ||
| expect(document.getElementById(id!)).not.toBeNull(); | ||
| unmount(); | ||
| expect(document.getElementById(id!)).toBeNull(); | ||
| }); | ||
| }); |
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.
in the case that multiple copies are loaded, this could create conflicting ids, better to use id generation like
crypto.randomUUIDThere 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.
or put the id generation into the hooks and make use of
useId