[Feature Request]: Add option to supress ref error for component using downshift

gcattan commented 2 years ago

Summary

We have the following downshift error in the console:

This issue seems well documented in the downshift repo:

https://github.com/downshift-js/downshift/issues/604 https://github.com/downshift-js/downshift/issues/1272

I would like to know if it is possible to add a supressRefError option in the component based on downshift:

Justification

No response

Desired UX and success metrics

No response

Required functionality

No response

Specific timeline issues / requests

No response

Available extra resources

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct

sstrubberg commented 2 years ago

Can you give a link to reproduce this? Possibly in a codesandbox? Thanks!

gcattan commented 2 years ago

Hi, I am really sorry but I cannot make it work in the sandbox. This is happenning when the component is loaded with react-test-renderer. Here is the closest configuration I achieved (but still, not showing the right error).

However, in my local environment, I can reproduce this error. To solve it, I needed to implement the same workaround you did for the getRootProps in Combobox.js:

... but for the getMenuProps function (also in Combobox.js but likely need to be propagated to all of these components):

-> modified direclty in node_module -> OK

tay1orjones commented 2 years ago

@gcattan Thanks for providing more info. I know how hard it can be to get a working repro up and going.

Our testing setup uses react-test-renderer as well and we've not hit this issue before that I'm aware of. Unless we've got a really solid reason as to why this is happening, I don't want to blindly set suppressRefError: true across all usages of getMenuProps, it leaves a big hole for other potential issues. From the downshift docs:

In some cases, you might want to completely bypass the refKey check. Then you can provide the object {suppressRefError : true} as the second argument to getMenuProps. Please use it with extreme care and only if you are absolutely sure that the ref is correctly forwarded otherwise Downshift will unexpectedly fail.

Based on the issues you linked, is it possible in your project a component somewhere isn't properly forwarding a ref or something? I'm not sure how to narrow down what about your project's configuration is causing this issue vs others and not seeing this before in our own project either.

gcattan commented 2 years ago

If we log the current reference on the componentDidMount, the error is now reproducible:

My understanding is that refs takes a variable amount of time to be set when testing, even when mounted by the react-dom. This does not happen in production code.

Do you think we can expose suppressRefError at the carbon components' level?

tay1orjones commented 2 years ago

Do you think we can expose suppressRefError at the carbon components' level?

@gcattan Yeah, providing this as an option via downshiftProps or as a new prop on the component makes sense. It's unlikely the core team will take this on anytime soon, but PRs are always welcome! 🙏

carbon-design-system / carbon