Make refs point to "correct" elements

[nlopin]

The outcome of the dialog #144

Example code: https://codesandbox.io/s/wave-input-refs-c5ure

Currently, the ref is attached to the wrapper which contradicts our agreement to attach to the most obvious element. We need to attach the ref to the input and research for other use cases needed to be fixed.

Components using forwardRef that need correction:

• Input should have an HTMLInputElement
• InnerInput should have an HTMLInputElement
• Password should have an HTMLInputElement

Components using other components internally that have changed refs:

• DatepickerRangeInput using Input
• PhoneInput using Input

[martimalek]

The draft PR ⬆️ solves the issue for Input and Password, Textarea is not forwarding a ref at the moment and Datepicker is forwarding one but it's not used AFAIK (check the PR for more info). Our agreement on refs talks about where to attach the ref to in a specific component but it does not say anything about in which components we should forward refs, should we do it in every form component? Should we do it only when necessary (when a user need arises)?

In my opinion we could just forward refs for the components where they are being used Input and Password and drop it everywhere else (since imperative behaviour with refs should be avoided in general). Please give me your opinions... 🙏🏽

[arturmiglio]

and drop it everywhere else (since imperative behaviour with refs should be avoided in general). Please give me your opinions... 🙏🏽

I'm not sure I agree with this.

Some doubts that maybe would also help others have a more informed opinion:

• What is the use case for ref in Input and Password (or any component for that matter)?
• Are there any downsides of forwarding the ref?

Maybe some benchmarking would also help us decide? How other component libraries deal with the same topic?

[martimalek]

I understand your concerns and agree that we lack some research to make an informed decision, I'll do the research and give an answer before the end of the week

[martimalek]

Hey, I haven't had a lot of time to take a deep look into this, but I wanted to give an answer before the end of the week to at least give some initial context for everyone new to the discussion, so here it goes.

First I want to make a small clarification, in this discussion we are not talking about general usage of refs in Wave, we are talking specifically about forwarding refs, meaning exposing an underlying DOM element so that a project using a Wave component can easily interact with said element (e.g. we can forward a ref in the Input component so that a project using it can directly interact with the html input underneath).

The imperative (ref.current = x) API offered by refs probably does not sit well with our declarative selfs, actually even React tells us to treat them with care:

Treat refs as an escape hatch. Refs are useful when you work with external systems or browser APIs. If much of your application logic and data flow relies on refs, you might want to rethink your approach.

but even though we should use them sparingly, sometimes they are simply necessary, here are some still valid examples from the legacy React docs:

There are a few good use cases for refs:

• Managing focus, text selection, or media playback.
• Triggering imperative animations.
• Integrating with third-party DOM libraries.

And to add a specific example from Wave take a look at this PR in the Password component, where the intention was to focus the underlying input element when toggling the "eye" button to show/hide the password, but since we were passing the ref to the wrapper div instead of to the input the only solution was to find the input using querySelector.

As for downsides of refs I haven't really found any other than the shift in paradigm, there is a Github issue in React with some benchmarks on forwardRef but it gets closed due to being stale without really concluding whether forwarding the ref is really a performance problem or not.

With all this being said, I think we need two agreements related to forwarding refs:

Which underlying DOM elements should we attach a ref to when we forward it?

This one is actually easier to answer since we already had an "agreement" to always attach to the most obvious element (e.g. Input component attaches to the underlying html input element, "agreement" is using double quotes because that is an open issue and this agreement does not appear in our live documentation).

Which components should we forward refs on?

It's entirely up to us, I don't have a strong opinion about this but I think ideally we should allow to pass refs only to elements that have behaviour that can't be invoked declaratively (e.g. input.focus()), I haven't found a resource online that lists these cases, so the only way that I've come up with to get to this point is to simply "wait" for requests to come to us in the form of issues, analyse case by case whether the request can only be fulfilled using a ref and if that is the case forward the ref in said component. Since this can be quite a hassle, I'm entirely open to allowing to pass refs to every component. Actually this seems to be a common approach:

• MUI allows to pass refs to all components (to the outermost element)
• Chakra UI has it's own wrapper of React.forwardRef and is used extensively in their codebase (they don't have documented which pattern they are following to allow the ref or not though, or at least I didn't find it)

Of course this is simply my personal opinion and I'd really like to hear some others 🤗

[martimalek]

I'll close this issue after merging #389, in that PR we are following our agreement on refs for the components that were already forwarding the ref (Input, InnerInput, Password), apart from that I'm removing the forwardRef from our internal Datepicker component since it wasn't being used.