-
Notifications
You must be signed in to change notification settings - Fork 51
Tag - Fix performance issues with single ResizeObserver #3033
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git โ๏ธ
|
b3dea53
to
839a3dd
Compare
839a3dd
to
815bbb7
Compare
@@ -33,23 +34,44 @@ export interface HdsTagSignature { | |||
Element: HTMLSpanElement; | |||
} | |||
|
|||
const overflowed = new TrackedWeakSet<Element>(); |
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.
This is the key to getting this working - externalizing the observer is only possible if we can track what elements are overflowed. To do this with ember's tracking, tracked-built-ins
provides a tracking-entangled WeakSet
. This means that getters and other computed properties that rely on this WeakSet will recompute when items are added or removed.
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.
Checked the performance times for before and after and this fix correct the issues greatly. Will add some screenshots to the PR description for visibility.
I think the Percy failure should go away once the showcase app commit is dropped. |
@dchyun I've added you as reviewer, since you have more context about this feature. |
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.
@@ -33,23 +34,44 @@ export interface HdsTagSignature { | |||
Element: HTMLSpanElement; | |||
} | |||
|
|||
const overflowed = new TrackedWeakSet<Element>(); |
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.
Checked the performance times for before and after and this fix correct the issues greatly. Will add some screenshots to the PR description for visibility.
@dchyun thanks for taking a look! And feel free to pick it up - let me know if I can help or if you have other questions. |
815bbb7
to
d7ecfeb
Compare
f554fb6
to
1a18adb
Compare
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.
Pull Request Overview
This PR refactors the Hds::Tag
component to use a single shared ResizeObserver
instead of creating one observer per tag instance, significantly improving rendering performance when displaying large numbers of tags (reducing loading time from 60s to 5s in reported cases).
- Replaced per-instance ResizeObserver with a single shared observer
- Added TrackedWeakSet to manage overflow state across all tag instances
- Created performance demonstration page with 1000 tag instances
Reviewed Changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/components/src/components/hds/tag/index.ts | Core refactoring to use shared ResizeObserver and TrackedWeakSet for overflow tracking |
packages/components/package.json | Added tracked-built-ins dependency |
showcase/app/templates/page-components/tag/frameless/demo-observer-performance.hbs | Demo template rendering 1000 tags for performance testing |
showcase/app/routes/page-components/tag/frameless/demo-observer-performance.js | Route generating test data array of 1000 items |
showcase/app/templates/page-components/tag/index.hbs | Added link to performance demo |
showcase/app/router.ts | Added nested route for frameless demo |
.changeset/hungry-meals-serve.md | Changelog entry documenting the performance fix |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
if (this._element) { | ||
observer.unobserve(this._element); | ||
} | ||
delete this._element; |
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.
Using delete
operator on object properties can impact performance and prevent optimizations. Consider setting this._element = undefined
instead.
delete this._element; | |
this._element = undefined; |
Copilot uses AI. Check for mistakes.
OK I take back what I said about the overflow being weird, that demo / iframe in the showcase app looks great โค๏ธ |
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.
left a couple of suggestions
๐ Summary
This PR changes
Hds::Tag
to use a singleResizeObserver
for all instances ofHds:Tag
. This drastically improves performance when rendering the component in large arrays.๐ ๏ธ Detailed description
๐ This stemmed from an issue reported in Slack where users noted that when rendering a decently sized array of strings, changing from
Hds::Tag
toHds::Badge
drastically reduced the rendering time (60s to 5s - note: this also included all of the network calls, etc.).Looking at the difference between those two components, the major difference is that the
Hds::Tag
adds a newResizeObserver
for each instance of the component. Refactoring the component so that only a singleResizeObserver
is needed for all instances greatly improves the initial rendering performance.NOTE: The best way to test this is to drop all but the first commit, and load up the
Tag
page in the showcase app which now renders a large list of tags. This will show the slow loading issue with the current implementation. Then adding in the second commit (or looking at the preview deploy) will show how the refactor improves the performance of the large list.SECOND NOTE (๐): The first commit should be dropped if / when you want to merge this.
๐ธ Screenshots
Tag showcase page loading time w/ new example
Before

After

๐ External links
Jira ticket: HDS-XXX
Figma file: [if it applies]
๐ Component checklist
๐ฌ Please consider using conventional comments when reviewing this PR.
๐ PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.