MM-69835 Remove all usage of findDOMNode - #11
Conversation
hmhealey
changed the title
There was a problem hiding this comment.
The current versions of react-overlays and react-transition-group both use findDOMNode, so I've had to update them. In some cases, they still might try to call it if we pass a React element as a prop instead of an HTMLElement, but my hope is that we only ever use them through react-bootstrap so that won't happen.
The changes here are just porting the previous patch over to the new version of react-overlays. Note that this patch doesn't actually affect the code used at runtime, and the web app is going to actually have its own patch for this library
| return <div {...props} className={classNames(className, classes)} />; | ||
| return ( | ||
| <div | ||
| ref={this.containerRef} |
There was a problem hiding this comment.
Some of these are straightforward because there's already a DOM node that we can attach a ref to instead of using findDOMNode
| const handleExit = createChainedFunction(this.handleExit, onExit); | ||
| const handleExiting = createChainedFunction(this.handleExiting, onExiting); | ||
|
|
||
| const ref = makeMergedRef([this.childRef, getElementRef(children)]); |
There was a problem hiding this comment.
This is a case where we need to inject a ref and hope that it returns a DOM node. I think this may cause an error if Transition doesn't forward its ref to an HTML element, but I don't think we pas anything as Transition other than Fade and Collapse, both of which now forward refs
|
|
||
| focus() { | ||
| const toggle = ReactDOM.findDOMNode(this.toggle); | ||
| const toggle = this.containerRef.current.querySelector( |
There was a problem hiding this comment.
In this case, I didn't think we can pass a ref around for this, so I went with searching the DOM and hoping that the a11y for the toggle button is set up correctly. We use this component in 1 place in the web app (which has tests thankfully) and one or two plugins, so I'm hoping this doesn't break
| } | ||
| } | ||
|
|
||
| function RootCloseWrapper({ |
There was a problem hiding this comment.
The new version of react-overlays doesn't have its own RootCloseWrapper, and I think it's too much work to migrate this to a functional component
| function DialogTransition(props) { | ||
| return <Fade {...props} timeout={Modal.TRANSITION_DURATION} />; | ||
| } | ||
| const DialogTransition = React.forwardRef((props, ref) => ( |
There was a problem hiding this comment.
These forwardRefs aren't actually needed once we're using React 19, but I wanted to make sure this all works with React 18 to ease in the upgrade
| prefix(props, 'backdrop'), | ||
| backdropClassName, | ||
| inClassName | ||
| renderBackdrop={backdropProps => ( |
There was a problem hiding this comment.
There's a change in the API for the react-overlays Modal because:
- The
backdropClassNameprop was replaced byrenderBackdrop - Instead of passing a child that gets cloned to inject props, there's now a
renderDialogprop.
I think I've got them both working the same now since I didn't want to change the API of our React Bootstrap components at all
| bsClass: PropTypes.string, | ||
| bsSize: PropTypes.oneOf(SIZES) |
There was a problem hiding this comment.
These should've been in #12, so just ignore them for now
| {child} | ||
| <BaseOverlay {...props} target={null} transition={transition}> | ||
| {({ props: overlayProps }) => ( | ||
| <div ref={overlayProps.ref} style={{ display: 'content' }}> |
There was a problem hiding this comment.
In this case, I didn't have an option for attaching a ref to the arbitrary child or using a11y attributes, so I instead opted to add an extra div and just hope that it can properly match the size of its child by using display: content
|
@coderabbitai Review this |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe change updates overlay dependencies and Escape handling. It replaces ChangesReact ref and overlay modernization
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
package.jsonParsing error: Unexpected token, expected ";" 1 | {
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Collapse.js (1)
201-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the DOM node in
Collapsetransition callbacks.With
react-transition-group@4.4.5,nodeRefomits the DOM node. Enter callbacks receiveisAppearing, exit callbacks receiveundefined, andaddEndListenerreceives onlydone. Wrap all exposed callbacks to passthis.childRef.current, and add regression tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Collapse.js` around lines 201 - 216, Update the callback wrappers in Collapse’s transition setup to prepend this.childRef.current to onEnter, onEntering, onEntered, onExit, onExiting, and addEndListener arguments, preserving isAppearing for enter callbacks and undefined for exit callbacks. Ensure consumers receive the DOM node despite nodeRef, and add regression coverage for these callback signatures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 129: Update TabPane’s caller-supplied transition rendering to create and
pass a DOM nodeRef to the transition, and attach that ref to the pane element so
custom transitions avoid findDOMNode. Preserve the existing Fade, Collapse, and
modal transition behavior, while requiring custom transition components to
consume the provided nodeRef.
In `@src/Fade.js`:
- Around line 72-101: Update the Fade transition callbacks around the Transition
usage to restore the DOM-node first argument omitted when nodeRef is provided.
Wrap onEnter, onEntering, onEntered, onExit, onExiting, onExited, and
addEndListener so each invokes the supplied callback with childRef.current first
while preserving remaining arguments and existing behavior, including
Modal/DialogTransition forwarding. Add regression coverage for enter, exit, and
addEndListener callbacks.
In `@src/Modal.js`:
- Around line 279-294: Restore compatibility for dialogComponentClass values
that do not forward refs by preserving the previous wrapper behavior around the
Dialog render path. Ensure this._modal.dialog consistently references the
expected DOM dialog element so updateStyle() works when the modal enters, while
retaining support for ref-forwarding components and existing styling props.
In `@src/Overlay.js`:
- Around line 91-96: Update the BaseOverlay invocation and rendered overlay
element in Overlay: remove target={null} so triggerRef is passed through, and
spread the generated overlayProps onto the div alongside its ref and existing
style. Preserve the child rendering and current display style.
In `@src/OverlayTrigger.js`:
- Around line 297-299: Update the trigger rendering in OverlayTrigger so
triggerRef attaches to the actual geometry-bearing trigger DOM node via ref
forwarding or the established render-prop contract, rather than relying on
invalid display:'content'. If arbitrary children require a wrapper, give it an
explicit measurable inline layout contract and preserve trigger positioning; add
a regression test covering inline-trigger geometry.
In `@src/utils/mergeRefs.js`:
- Around line 10-20: Update makeMergedRef to retain cleanup functions returned
by callback refs, and return a composed cleanup that clears object refs, invokes
each returned cleanup, and calls ref(null) only for callbacks without cleanup.
Ensure useMergedRef uses this updated behavior without discarding the returned
cleanup.
---
Outside diff comments:
In `@src/Collapse.js`:
- Around line 201-216: Update the callback wrappers in Collapse’s transition
setup to prepend this.childRef.current to onEnter, onEntering, onEntered,
onExit, onExiting, and addEndListener arguments, preserving isAppearing for
enter callbacks and undefined for exit callbacks. Ensure consumers receive the
DOM node despite nodeRef, and add regression coverage for these callback
signatures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bad325a-fb9c-481e-a107-91abbddd8205
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
package.jsonpatches/react-overlays+0.9.3.patchpatches/react-overlays+5.2.1.patchsrc/CarouselItem.jssrc/Collapse.jssrc/Dropdown.jssrc/DropdownMenu.jssrc/Fade.jssrc/Modal.jssrc/ModalDialog.jssrc/Nav.jssrc/Overlay.jssrc/OverlayTrigger.jssrc/utils/mergeRefs.jstest/DropdownSpec.jstest/FadeSpec.jstest/ModalSpec.jstest/OverlayTriggerSpec.jstest/index.js
💤 Files with no reviewable changes (3)
- test/DropdownSpec.js
- test/index.js
- patches/react-overlays+0.9.3.patch
I removed the container prop from Modal, but we don't seem to use that ourselves so it's probably fine to do. It was marked as private anyway.
This used to come from react-overlays, but it's not part of the newer version.
- Fix TabPane transition - Fix callbacks to transitions to pass element as first parameter - Remove dialogComponentClass prop from Modal
…reaking change) Breaking change: overlay prop of OverlayTrigger and child of Overlay must forward ref to a DOM node. It will cause tooltips to either not appear or not be correctly positioned in affected plugins. The only one of our plugins that should be affected is mattermost-plugin-legal-hold. To fix it, its Tooltip must be updated to forward a ref to the inner RBTooltip, and OverlayTrigger must similarly pass a ref to the child of OverlayWrapper.
This fixes my previous attempt because the div didn't have a box to get the dimensions for by giving it dimensions. I'm not sure if this will work 100% because the child element can be either inline or block, but this seems to work based on some brief testing. To fully match the old behaviour, we'd attach the ref to the cloned child, but that would be a breaking change to a few plugins.
2 participants
Summary
The last big change to get this working with React 19 is to remove usage of findDOMNode. This is also the one that's the most likely to be a breaking change because it makes it so that we need to pass HTML elements around or use
querySelectorto get them which I did in a few places.I'm mostly relying on the tests in mattermost/mattermost#37758 to ensure that I haven't broken anything here, so fingers crossed we don't run into any issues with how plugins or anything else use these components.
Ticket Link
https://mattermost.atlassian.net/browse/MM-69835