[#77] Finally drop use of findDOMNode()

stefcameron · stefcameron · commit 4e6929c733ad · 2022-06-09T19:06:51.000-05:00
Fixes #77 Fixes #704 For the longest time, since before I took over, we were filtering specified containers through `ReactDOM.findDOMNode()`, but why? The configured trap containers (via the `containerElements` prop) must be elements already, which means they must already be rendered, which means there's no point in passing them through `findDOMNode()` to find an underlying DOM element because they aren't React elements in the first place. The removal of this call should mean that React Strict Mode will finally be OK with focus-trap-react. Note it's still possible to render a focus trap with a single React element child. That works fine, and never needed `findDOMNode()` anyway. We were already using a callback ref to get its element to then auto-configure that element as the single container for the focus trap. Just to be sure, however, this will be released as a new __major__ version.
diff --git a/.changeset/shy-terms-fold.md b/.changeset/shy-terms-fold.md
@@ -0,0 +1,5 @@
+---
+'focus-trap-react': major
+---
+
+Stop using the infamous `findDOMNode()` on provided `containerElements`. There seems to have been no good reason for this as this prop, if specified, is already required to be an array of HTMLElement references, which means these nodes have already been rendered (if they were once React elements). There appears to have been no remaining need for this API. Furthermore, the minimum supported version of React is now 16.3 as it technically has been for a while now since that is the version that introduced callback refs, which we've been using for quite some time now (so this bump shouldn't cause any ripples).
diff --git a/package.json b/package.json
@@ -99,7 +99,7 @@
   },
   "peerDependencies": {
     "prop-types": "^15.8.1",
-    "react": ">=16.0.0",
-    "react-dom": ">=16.0.0"
+    "react": ">=16.3.0",
+    "react-dom": ">=16.3.0"
   }
 }
diff --git a/src/focus-trap-react.js b/src/focus-trap-react.js
@@ -1,14 +1,8 @@
 const React = require('react');
-const ReactDOM = require('react-dom');
 const PropTypes = require('prop-types');
 const { createFocusTrap } = require('focus-trap');
 const { isFocusable } = require('tabbable');
 
-// TODO: These issues are related to older React features which we'll likely need
-//  to fix in order to move the code forward to the next major version of React.
-//  @see https://github.com/davidtheclark/focus-trap-react/issues/77
-/* eslint-disable react/no-find-dom-node */
-
 class FocusTrap extends React.Component {
   constructor(props) {
     super(props);
@@ -277,18 +271,11 @@ class FocusTrap extends React.Component {
 
   setupFocusTrap() {
     if (!this.focusTrap) {
-      const focusTrapElementDOMNodes = this.focusTrapElements.map(
-        // NOTE: `findDOMNode()` does not support CSS selectors; it'll just return
-        //  a new text node with the text wrapped in it instead of treating the
-        //  string as a selector and resolving it to a node in the DOM
-        ReactDOM.findDOMNode
-      );
-
-      const nodesExist = focusTrapElementDOMNodes.some(Boolean);
+      const nodesExist = this.focusTrapElements.some(Boolean);
       if (nodesExist) {
         // eslint-disable-next-line react/prop-types -- _createFocusTrap is an internal prop
         this.focusTrap = this.props._createFocusTrap(
-          focusTrapElementDOMNodes,
+          this.focusTrapElements,
           this.internalOptions
         );
 
@@ -378,7 +365,7 @@ class FocusTrap extends React.Component {
         );
       }
 
-      const composedRefCallback = (element) => {
+      const callbackRef = (element) => {
         const { containerElements } = this.props;
 
         if (child) {
@@ -395,7 +382,7 @@ class FocusTrap extends React.Component {
       };
 
       const childWithRef = React.cloneElement(child, {
-        ref: composedRefCallback,
+        ref: callbackRef,
       });
 
       return childWithRef;

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +'focus-trap-react': major
 +---
++
 +Stop using the infamous `findDOMNode()` on provided `containerElements`. There seems to have been no good reason for this as this prop, if specified, is already required to be an array of HTMLElement references, which means these nodes have already been rendered (if they were once React elements). There appears to have been no remaining need for this API. Furthermore, the minimum supported version of React is now 16.3 as it technically has been for a while now since that is the version that introduced callback refs, which we've been using for quite some time now (so this bump shouldn't cause any ripples).
Original file line number	Diff line number	Diff line change
`@@ -99,7 +99,7 @@`
`99`	`99`	`},`
`100`	`100`	`"peerDependencies": {`
`101`	`101`	`"prop-types": "^15.8.1",`
`102`		`- "react": ">=16.0.0",`
`103`		`- "react-dom": ">=16.0.0"`
	`102`	`+ "react": ">=16.3.0",`
	`103`	`+ "react-dom": ">=16.3.0"`
`104`	`104`	`}`
`105`	`105`	`}`