[Link] TypeError: Cannot read property 'addEventListener' of undefined

[7s4r]

When I'm running unit tests with Jest, I get this error:
TypeError: Cannot read property 'addEventListener' of undefined

I think this is related to this issue and this pull request. As breadcrumb component was added recently from lab to core, it is missing fallback to ownerWindow.

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Unit test should pass without errors

Current Behavior 😯

Unit test fail with this stack trace:

TypeError: Cannot read property 'addEventListener' of undefined
 at prepare (node_modules/@material-ui/core/utils/focusVisible.js:94:17)
      at node_modules/@material-ui/core/utils/focusVisible.js:147:7
      at setRef (node_modules/@material-ui/core/utils/reactHelpers.js:20:5)
      at node_modules/@material-ui/core/utils/reactHelpers.js:39:7
      at commitAttachRef (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9593:7)
      at commitAllLifeCycles (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10834:7)
      at HTMLUnknownElement.callCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:2377:14)
      at Object.invokeGuardedCallbackDev (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:2427:16)
      at invokeGuardedCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:2480:31)
...

Steps to Reproduce 🕹

Link: https://codesandbox.io/s/create-react-app-9e5kw

1. Launch unit test with jest

Context 🔦

I was trying to unit test with Jest my component.

Your Environment 🌎

Tech	Version
Material-UI	v4.1.0
React	v16.8.0
Browser	N/A (in linux terminal)
Node	v10.15.3

[oliviertassinari]

@7s4r This is mostly a documentation issue. You should check https://material-ui.com/components/links/#third-party-routing-library regarding how react-router should be used with the Link component.

However, I believe we can improve the DX around this issue. I would propose the following diff:

diff --git a/packages/material-ui/src/Link/Link.js b/packages/material-ui/src/Link/Link.js
index e82c8b0e3..f559a9621 100644
--- a/packages/material-ui/src/Link/Link.js
+++ b/packages/material-ui/src/Link/Link.js
@@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import { capitalize } from '../utils/helpers';
 import withStyles from '../styles/withStyles';
+import { elementTypeAcceptingRef } from '@material-ui/utils';
 import { useIsFocusVisible } from '../utils/focusVisible';
 import { useForkRef } from '../utils/reactHelpers';
 import Typography from '../Typography';
@@ -142,7 +143,7 @@ Link.propTypes = {
    * The component used for the root node.
    * Either a string to use a DOM element or a component.
    */
-  component: PropTypes.elementType,
+  component: elementTypeAcceptingRef,
   /**
    * @ignore
    */
diff --git a/packages/material-ui/src/utils/focusVisible.js b/packages/material-ui/src/utils/focusVisible.js
index f20ddb884..6cae7319a 100644
--- a/packages/material-ui/src/utils/focusVisible.js
+++ b/packages/material-ui/src/utils/focusVisible.js
@@ -127,6 +127,15 @@ export function useIsFocusVisible() {
   const ref = React.useCallback(instance => {
     const node = ReactDOM.findDOMNode(instance);
     if (node != null) {
+      if (!node.ownerDocument) {
+        throw new Error(
+          [
+            'Material-UI: the ref forwarding logic is not correct.',
+            `An Element is expected but found ${node}.`,
+          ].join('\n'),
+        );
+      }
+
       prepare(node.ownerDocument);
     }
   }, []);

cc @eps1lon.

Do you want to submit a pull request? :)

[eps1lon]

Though we don't need the manual prepare step. This can be delegated to useIsFocusVisible.

[7s4r]

Thank you @oliviertassinari ! It works adding this code:

const AdapterLink = React.forwardRef((props, ref) => <RouterLink innerRef={ref} {...props} />)

[oliviertassinari]

@eps1lon The ButtonBase component fails identically to the Link component. I believe the components delegate the preparation step to when the ref triggers.

[eps1lon]

Yep which makes more sense in a concurrent environment or non-ref environment. Not sure what you're trying to say. Breadcrumbs just needs to use the full useIsFocusVisible hook.

[oliviertassinari]

@eps1lon What do you mean by full hook? The error reported in this issue originate from the Link component. Why is the breadcrumb a variable of the equation?

[eps1lon]

Why is the breadcrumb a variable of the equation?

[Breadcrumb] TypeError: Cannot read property 'addEventListener' of undefined #16454

What do you mean by full hook?

I was under the impression that patch targeted Link which was still using prepare. There's no need to add an additional error in the prepare logic. We already have error messages for incorrect ref forwarding. Additional ones are just noise at this point.

[oliviertassinari]

@eps1lon I wish elementTypeAcceptingRef was enough in this case. The custom prop type warns for

<Button component={(props) => props.children}>

but doesn't warn for

import { Link as RouterLink } from "react-router-dom";

<Button component={RouterLink}>

---

No matter what,

diff --git a/packages/material-ui/src/Link/Link.js b/packages/material-ui/src/Link/Link.js
index e82c8b0e3..f559a9621 100644
--- a/packages/material-ui/src/Link/Link.js
+++ b/packages/material-ui/src/Link/Link.js
@@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import { capitalize } from '../utils/helpers';
 import withStyles from '../styles/withStyles';
+import { elementTypeAcceptingRef } from '@material-ui/utils';
 import { useIsFocusVisible } from '../utils/focusVisible';
 import { useForkRef } from '../utils/reactHelpers';
 import Typography from '../Typography';
@@ -142,7 +143,7 @@ Link.propTypes = {
    * The component used for the root node.
    * Either a string to use a DOM element or a component.
    */
-  component: PropTypes.elementType,
+  component: elementTypeAcceptingRef,
   /**
    * @ignore
    */

Would already be a great step forward, @7s4r si tu veux contribuer au projet :) ?

[eps1lon]

Huh that's a weird one. I guess it doesn't warn because it's a class component at which point I don't understand why we can't get the correct host instance (that's what the findDOMNode fallback is for).

I can't get the codesandbox to run (had to add 2 missing dependencies and then it failed in jss).

Your patch is a great step but I'm interested in a reproducible example for that error message.

[oliviertassinari]

@eps1lon For reproducing, I had to use the export as zip feature of Codesandbox:

It seems that findDOMNode returns a weird object with import renderer from "react-test-renderer";

[eps1lon]

Yeah so it's an issue with react-test-renderer. No warning for <Button component={RouterLink}> is correct because RouterLink is a class component.

It's a slight variation of the previous issue where we prepared in the effect phase. In react-test-renderer there are no host instances which means we could fix it by moving it into the ref phase. This doesn't get called for host components but for class components it does happen.

Not sure if proper ref forwarding would fix it but adding an error there wouldn't solve the issue (because the usage is perfectly fine). We would need to silence it of which I am no fan.

I would need to look into why findDOMNode returns an object in react-test-renderer. If it doesn't return host instances in refs it should also not return something for findDOMNode.

[oliviertassinari]

@eps1lon I think that you are right. I would suspect a bug with findDOMNode and react-test-renderer .toJSON().

[Breizhad]

<Link component={(props) => <ReactRouterLink {...props} />} work for me

[icancode007]

I can take a shot at this if its ok with you

[oliviertassinari]

@icancode007 Sorry, we already have @Nikhil-Pavan-Sai on it.