Skip to content
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

fix(Ref) Add a workaround for findDomNode to work with React 16.6.0 #3258

Closed

Conversation

laplacesdemon
Copy link

This is a workaround for the following issue: #3255

React 16.6.0 throws an error if findDOMNode is called on an unmounted component which is the case in enzyme's shallow component testing.

Please consider this change as a temporary workaround until there's a better way to handle findDOMNode. React states that it's an antipattern to use it in componentDidMount.

@welcome
Copy link

welcome bot commented Nov 5, 2018

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
  • Run yarn test locally to catch errors. This ensures all components still behave as they should.
  • Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
  • Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

// eslint-disable-next-line react/no-find-dom-node
if (innerRef) innerRef(findDOMNode(this))
// eslint-disable-next-line no-empty
} catch (_) {}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silencing the error is horrible and I'm really not happy with this line. There are 2 options: either report it with console.error which will pollute the unit test runs, or only do a try catch in development environment (i.e. check for process.env.NODE_ENV === 'development). What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@layershifter Given your updates in Stardust, what's your thought here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will backport changes from microsoft/fluent-ui-react#491, this will resolve this issue fully 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this issue.

@layershifter
Copy link
Member

layershifter commented Nov 6, 2018

I almost sure that there is a different issue, need to check it. If you will check our package.json you will see that we're using React 16.6:

"react": "^16.6.0",
"react-dom": "^16.6.0",

And our test suite passed for it.

@laplacesdemon
Copy link
Author

@layershifter you won't hit this issue if you don't do snapshot testing via react-test-renderer. If your unit tests don't have that, then it's OK to use React 16.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants