-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversation
💖 Thanks for opening this pull request! 💖 Here is a list of things that will help get it across the finish line:
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 (_) {} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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.
I almost sure that there is a different issue, need to check it. If you will check our "react": "^16.6.0",
"react-dom": "^16.6.0", And our test suite passed for it. |
@layershifter you won't hit this issue if you don't do snapshot testing via |
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 inenzyme
'sshallow
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 incomponentDidMount
.