validateGetRootPropsCalledCorrectly does not correctly check if the root props are applied

[yp]

• downshift version: 1.14.0
• node version:
• npm (or yarn) 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.

[tansongyang]

I believe the problem is that you are not using refKey. Please see the documentation on getRootProps.

Change MyDiv like so:

const MyDiv = ({innerRef, ...rest}) => <div ref={innerRef} {...rest} />

And then change your call site:

<MyDiv {...getRootProps({refKey: 'innerRef'})}>
   {/* ... */}
</MyDiv>

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]

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]

OK. Is there no way to make refKey work in your scenario?

[yp]

What about transforming the throw new Errors into console.error and surrounding the function call by a if (NODE_ENV!=='production') ?

[kentcdodds]

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]

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]

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]

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 Error transformed in console.error. I think that the example correctly works, but maybe I am missing something.

[yp]

The main point here is that the getRootProps call 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 root div. Since the ref is 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]

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 refKey and spreading on a composite component.

What I'm thinking is that we could add an option you could pass to getRootProps that is: silenceRefError: true or 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]

Reopening since this conversation is taking a different direction.

[kentcdodds]

Maybe instead of silenceRefError it should be suppressRefError

[yp]

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...