Repository navigation
validateGetRootPropsCalledCorrectly does not correctly check if the root props are applied #235
Description
downshiftversion:1.14.0nodeversion:npm(oryarn) version:
Reproduction repository:
https://codesandbox.io/s/23yzon246r
Problem description:
I would like to use a non-DOM component as root component returned by the children function. I pass to it a prop rootProps containing the result of getRootProps() and it spreads the object to the inner root div. In such a way, the root props that Downshift needs are correctly applied. However the validateGetRootPropsCalledCorrectly throws an error.
Suggested solution:
I would suggest to either remove the check or to add a prop excludeSafetyChecks that allows to bypass such a check.
Activity
tansongyang commentedon Oct 20, 2017
I believe the problem is that you are not using
refKey. Please see the documentation on getRootProps.Change
MyDivlike so:And then change your call site:
Here's a working example: https://codesandbox.io/s/m4n2416l1x
Closing now, as I believe this resolves the issue. If not, let me know and we can keep discussing.
yp commentedon Oct 20, 2017
In my case I cannot change the definition of MyDiv. Actually, the real case is more complicated than the example (that I greatly simplified in the codesandbox).
tansongyang commentedon Oct 20, 2017
OK. Is there no way to make
refKeywork in your scenario?yp commentedon Oct 20, 2017
What about transforming the
throw new Errors intoconsole.errorand surrounding the function call by aif (NODE_ENV!=='production')?kentcdodds commentedon Oct 20, 2017
If we did that then the component would just not work at all. Failing early and obviously is better. Any reason you couldn't wrap
<MyDiv />in a<div />like this?yp commentedon Oct 22, 2017
I understand your position, but the check is too simplicistic and it is failing even if the component would work because the ref is set correctly to the root component.
IMHO a failure when the component should not be failing is an issue, but if you do not agree, I will find a workaround.
kentcdodds commentedon Oct 22, 2017
But the component doesn't work. With the way refs work, downshift can't get the root DOM node without being able to pass a ref prop to the node itself. It's not enough to pass a ref to a composite component.
Unless I'm mistaken... Feel free to prove me wrong with a pull request.
yp commentedon Oct 23, 2017
Please refer to this example: https://codesandbox.io/s/n3jv69ywnl
It is the same example as the first repository but with the Downshift source code embedded and two
throw new Errortransformed inconsole.error. I think that the example correctly works, but maybe I am missing something.yp commentedon Oct 23, 2017
The main point here is that the
getRootPropscall return an object that is not spread onto the composite component but passed as-is to the root composite component which, internally, it spreads onto the rootdiv. Since therefis only a key of an object prop (and not a prop itself), react does not intercept it and it can pass to the "real" root DOM element.kentcdodds commentedon Oct 23, 2017
Ah, I see what you mean. Very interesting. Hmmm... I'm trying to think of a good way to keep the error but also allow you to do what you're doing there because I think that it's more likely people are making a mistake when you don't pass a
refKeyand spreading on a composite component.What I'm thinking is that we could add an option you could pass to
getRootPropsthat is:silenceRefError: trueor something like that. Then we'd update the error message to tell people they can use that if they know what they're doing. What do you think?tansongyang commentedon Oct 23, 2017
Reopening since this conversation is taking a different direction.
kentcdodds commentedon Oct 23, 2017
Maybe instead of
silenceRefErrorit should besuppressRefErroryp commentedon Oct 23, 2017
That's fine with me. And sorry if I was not sufficiently clear since the beginning.
If you want, I can try to prepare a PR along this line...
kentcdodds commentedon Oct 23, 2017
Yes please! Thank you!
Add the option `suppressRefError` to `getRootProps`.
feat: add the option `suppressRefError` to `getRootProps`. (#241)
feat: add the option `suppressRefError` to `getRootProps`. (downshift…
kumarabhirup commentedon Apr 5, 2019
suppressRefErrorstill gives me the same error :-(