Replace EuiObserver abstract class with useObserver hook#9511
Replace EuiObserver abstract class with useObserver hook#9511ragini-pandey wants to merge 13 commits intoelastic:mainfrom
EuiObserver abstract class with useObserver hook#9511Conversation
13b05a2 to
f7f685d
Compare
|
👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually? |
|
@ragini-pandey The 2 PRs you opened both migrate It would resolve:
We could link all of these in the PR description so they get automatically closed 😄 |
There was a problem hiding this comment.
Pull request overview
Refactors the observer utilities in EUI away from a shared abstract class (EuiObserver) into a shared hook (useObserver) as part of the class-to-hooks migration effort (issue #9458), while keeping the EuiResizeObserver and EuiMutationObserver public component APIs intact.
Changes:
- Replaced the
EuiObserverabstract base class with a newuseObserverhook that manages node ref updates and observer lifecycle/cleanup. - Converted
EuiResizeObserverandEuiMutationObserverfrom class components into function components that useuseObserver. - Added an upcoming changelog entry documenting the internal refactor.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/eui/src/components/observer/resize_observer/resize_observer.tsx | Converts EuiResizeObserver to a function component powered by useObserver and refs. |
| packages/eui/src/components/observer/observer.ts | Replaces the EuiObserver class with the useObserver hook and observer lifecycle management. |
| packages/eui/src/components/observer/mutation_observer/mutation_observer.tsx | Converts EuiMutationObserver to a function component powered by useObserver and refs. |
| packages/eui/changelogs/upcoming/9511.md | Adds a changelog entry for the observer refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the review! Both comments have been addressed in the latest commit: 1. JSDoc updated — The 2. Mount-time ref guard restored — Added an optional |
…#9458) Convert the abstract `EuiObserver` base class to a shared `useObserver` custom hook. Migrate both `EuiResizeObserver` and `EuiMutationObserver` from class components extending `EuiObserver` to function components that use the new hook. - Replace `EuiObserver` class with `useObserver` hook in `observer.ts` - Convert `EuiResizeObserver` to a function component using `useObserver` - Convert `EuiMutationObserver` to a function component using `useObserver` - All existing tests pass - No public API changes
- Update JSDoc to mention both EuiResizeObserver and EuiMutationObserver - Add optional componentName parameter to useObserver - Add mount-time guard (throws if no ref received), matching EuiObserver.componentDidMount behavior - Merge cleanup useEffect into the guard effect for clarity - Pass componentName from EuiResizeObserver and EuiMutationObserver callers
c185fff to
9fe935c
Compare
There was a problem hiding this comment.
Pull request overview
Refactors EUI’s internal observer components away from an abstract class pattern and into a shared hook, supporting the ongoing class-to-hooks migration effort (issue #9458) while keeping EuiResizeObserver/EuiMutationObserver behavior and exports intact.
Changes:
- Replaced the
EuiObserverabstract class with a shareduseObserver(beginObserve, componentName)hook. - Converted
EuiResizeObserverandEuiMutationObserverfrom class components to function components usinguseObserver. - Added an upcoming changelog entry describing the internal refactor.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/eui/src/components/observer/observer.ts | Introduces useObserver hook to manage ref callback + observer lifecycle/cleanup. |
| packages/eui/src/components/observer/resize_observer/resize_observer.tsx | Migrates EuiResizeObserver to hooks; uses refs to track prior size and latest callback. |
| packages/eui/src/components/observer/mutation_observer/mutation_observer.tsx | Migrates EuiMutationObserver to hooks; uses refs to keep latest mutation callback/options without cycling the ref callback. |
| packages/eui/changelogs/upcoming/9511.md | Documents the observer refactor for the next release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Refactors the observer utilities in packages/eui by replacing the EuiObserver abstract base class with a shared useObserver hook, and migrating EuiResizeObserver/EuiMutationObserver from class components to function components.
Changes:
- Replaced the
EuiObserverclass with a reusableuseObserver(beginObserve, componentName?)hook for ref + lifecycle management. - Converted
EuiResizeObserverandEuiMutationObserverto function components built onuseObserver. - Added an upcoming changelog entry documenting the refactor.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/eui/src/components/observer/observer.ts | Removes the abstract class and introduces the shared useObserver hook for observer lifecycle/ref handling. |
| packages/eui/src/components/observer/resize_observer/resize_observer.tsx | Migrates EuiResizeObserver to a hook-based function component and uses refs to avoid rerenders. |
| packages/eui/src/components/observer/mutation_observer/mutation_observer.tsx | Migrates EuiMutationObserver to a hook-based function component and stabilizes callback/options via refs. |
| packages/eui/changelogs/upcoming/9511.md | Adds changelog notes for the internal observer refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Add componentName to useEffect dependency array in useObserver, remove eslint-disable suppression - Remove React.FunctionComponent annotation from EuiMutationObserver, use typed destructured props instead - Remove as React.ReactElement type cast from EuiMutationObserver return statement
…nect behavior The old EuiObserver class called observer.disconnect() twice on unmount: once in componentWillUnmount and again via the ref callback (called with null). Each disconnect() calls clearTimeout in the MutationObserver polyfill, so the count was 9. The new useObserver hook only disconnects once (via the ref callback), then sets observerRef to null, so the useEffect cleanup is a no-op. This is the correct behavior. Update the expected count from 9 to 8.
aca833e to
466b6a2
Compare
There was a problem hiding this comment.
Pull request overview
Refactors EUI’s observer components by replacing the shared EuiObserver abstract class with a useObserver hook, and migrates EuiResizeObserver/EuiMutationObserver to function components as part of the broader class-to-hooks effort (issue #9458).
Changes:
- Added a shared
useObserverhook to manage ref binding, observer lifecycle, and cleanup. - Converted
EuiResizeObserverandEuiMutationObserverfrom class components to function components usinguseObserver. - Updated a popover cleanup test expectation and added an upcoming changelog entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/eui/src/components/observer/observer.ts | Replaces EuiObserver base class with the new useObserver hook and preserves the ref-on-mount guard behavior. |
| packages/eui/src/components/observer/resize_observer/resize_observer.tsx | Migrates EuiResizeObserver to a function component using useObserver and refs for dimension tracking. |
| packages/eui/src/components/observer/mutation_observer/mutation_observer.tsx | Migrates EuiMutationObserver to a function component using useObserver, keeping existing mutation observer helpers intact. |
| packages/eui/src/components/popover/popover.test.tsx | Updates timeout cleanup assertion to match new runtime behavior. |
| packages/eui/changelogs/upcoming/9511.md | Documents the internal refactor and component migrations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
@ragini-pandey additionally, as you can see above, our tests failed ☝🏻 specifically for |
|
|
||
| unmount(); | ||
| expect(window.clearTimeout).toHaveBeenCalledTimes(9); | ||
| expect(window.clearTimeout).toHaveBeenCalledTimes(8); |
There was a problem hiding this comment.
Is it expected that clearTimeout is called 8 times and not 9?
There was a problem hiding this comment.
Yes, this is expected and intentional. The count dropped from 9 → 8 because the old EuiObserver class disconnected the observer twice on unmount:
- In
componentWillUnmount— explicitly calledthis.observer.disconnect() - Via the ref callback — React calls the ref with
nullon unmount, which also triggeredupdateChildNode(null)→this.observer.disconnect()
Each disconnect() call invokes clearTimeout inside the MutationObserver polyfill (used in the test environment), so there were 2 disconnects = 2 extra clearTimeout calls, giving a total of 9.
The new useObserver hook disconnects only once — the updateChildNode ref callback handles disconnection when called with null on unmount. The useEffect cleanup also calls observerRef.current?.disconnect(), but by that point observerRef.current is already null (nulled out by the ref callback), making it a no-op. So there is now 1 observer disconnect = 1 clearTimeout, giving a total of 8.
This is the correct behavior — cleaning up once is better than twice, and the count change reflects that improvement.
|
@ragini-pandey since we are removing |
There was a problem hiding this comment.
Pull request overview
This PR continues the EUI class-to-hooks migration by replacing the internal abstract EuiObserver base class with a shared useObserver hook, and converting EuiResizeObserver/EuiMutationObserver to function components while preserving their public APIs.
Changes:
- Replaced
EuiObserverclass logic with a reusableuseObserver(beginObserve, componentName)hook that manages ref attachment, observer lifecycle, and cleanup. - Converted
EuiResizeObserverandEuiMutationObserverto function components usinguseObserver, using refs to avoid unnecessary re-renders. - Updated an
EuiPopovercleanup test expectation and added a changelog entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/eui/src/components/observer/observer.ts | Replaces the abstract class with the new useObserver hook and keeps the Observer interface. |
| packages/eui/src/components/observer/resize_observer/resize_observer.tsx | Refactors EuiResizeObserver to a function component backed by useObserver. |
| packages/eui/src/components/observer/mutation_observer/mutation_observer.tsx | Refactors EuiMutationObserver to a function component backed by useObserver. |
| packages/eui/src/components/popover/popover.test.tsx | Adjusts timeout cleanup assertion to match updated observer behavior. |
| packages/eui/changelogs/upcoming/9511.md | Records the internal refactor in the upcoming changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Hey @weronikaolejniczak, yarn — Completed successfully (with non-blocking peer dependency warnings) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
weronikaolejniczak
left a comment
There was a problem hiding this comment.
CI is failing because of formatting issues. Could you please address them? 🙏🏻 They're mentioned by the Copilot as well.
Hopefully after that the CI passes, I'll do some manual tests and we could be good to go!
weronikaolejniczak
left a comment
There was a problem hiding this comment.
Sorry, missclick 😅
…n_observer.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
💚 Build SucceededHistory
|
weronikaolejniczak
left a comment
There was a problem hiding this comment.
@ragini-pandey linter is still complaining because of formatting issues:
/app/packages/eui/src/components/observer/mutation_observer/mutation_observer.tsx
28:53 error Replace
EuiMutationObserverProps>·=·({⏎··children,⏎··onMutation,⏎··observerOptions,⏎with⏎··EuiMutationObserverProps⏎>·=·({·children,·onMutation,·observerOptions·prettier/prettier
Could you please address any issues on the PR 🙏🏻
- all unit tests with Jest should pass,
- all Cypress tests should pass,
- Storybook should run and stories should be tested manually,
- documentation website builds,
- there are no linter errors / formatting errors.
Additionally, could you add displayName to EuiMutationObserver and EuiResizeObserver? Thanks! 💚
💔 Build Failed
Failed CI StepsHistory
|
Summary
EuiObserverbase class with a shareduseObservercustom hook. Converted bothEuiResizeObserverandEuiMutationObserverfrom class components to function components.observer.ts: Replaced theEuiObserverclass with auseObserverhook that accepts abeginObservecallback (receives a DOM node, returns an observer instance). The hook manages the ref callback, observer lifecycle, and cleanup.resize_observer.tsx: ConvertedEuiResizeObserverfrom a class extendingEuiObserverto a function component usinguseObserver. Used refs instead ofsetStatefor tracking dimensions to avoid unnecessary re-renders.mutation_observer.tsx: ConvertedEuiMutationObserverfrom a class extendingEuiObserverto a function component usinguseObserver. The existinguseMutationObserverhook andmakeMutationObserverhelper are unchanged.Observerinterface andhasResizeObserverexports are preserved. No public API changes.API Changes
No public API changesScreenshots
N/A — No visual changes. This is an internal refactor only.
Impact Assessment
Impact level: 🟢 None
Release Readiness
Documentation: No docs changes needed (internal refactor)Figma: N/AMigration guide: N/A (no breaking changes)Adoption plan: N/A (internal refactor)QA instructions for reviewer
EuiResizeObserverstill works by checking components that use it (e.g., accordion, context menu panel, markdown editor, tooltip)EuiMutationObserverstill works by checking the popover componentcd packages/eui && yarn test-unit --testPathPattern=observercd packages/eui && npx tsc --noEmitChecklist before marking Ready for Review
Breaking changes: N/A