[Proposal] Automatically compose refs in cloneElement

[ide]

Currently cloneElement clobbers the original element's ref if you provide a new ref when cloning: https://github.com/facebook/react/blob/master/src/isomorphic/classic/element/ReactElement.js#L174. This is a proposal to automatically compose callback refs so that this works:

var original = <C ref={component => console.log('original ref') />;
var clone = React.cloneElement(original, {
  ref: component => console.log('clone ref');
});

// upon mounting prints:
// clone ref
// original ref

The implementation to compose refs would look similar to the technique used here: https://github.com/facebook/react-native/blob/fe9af98a2867475f5d4af9bbe49ce0ed0a7b1d08/Libraries/CustomComponents/Navigator/Navigator.js#L1041

var element = ...
var originalRef = element.ref;
if (originalRef != null && typeof originalRef !== 'function') {
  console.warn(
    'String refs are not composable. Use a callback ' +
    'ref instead. Ignoring ref: ' + originalRef
  );
  originalRef = null;
}

React.cloneElement(element, {
  ref: component => {
    // Whatever you want here

    if (originalRef) {
      originalRef(component);
    }
  }
});

When writing higher-order components I find myself composing refs a lot and having to write this ref-composition code several times. Easy to write a utility helper (https://github.com/ide/react-native-clone-referenced-element/blob/master/cloneReferencedElement.js) but thought this could make sense to build in with 0.14 approaching.

cc @spicyj @sebmarkbage

[jimfb]

@spicyj and @sebmarkbage are the correct people to figure this out, so we'll let them have the final word.

Question: If someone does want to replace the ref (for whatever reason, they don't want to preserve the original ref in any way and want to completely override it), how would they do this with the new API? I'm worried that this new version of cloneElement is strictly less flexible.

Personally, I feel like this might take us in the wrong direction for the core (I'd like to see refs get less special/magical over time, not more special/magical). I think it's great to have an easy way to wrap refs via a utility (you should consider publishing your version!), but I don't think it's good to have that be part of the core.

[ide]

If someone does want to replace the ref (for whatever reason, they don't want to preserve the original ref in any way and want to completely override it), how would they do this with the new API?

It's not possible in this version but I guess it's fair to want an escape hatch. My thinking is that most of the time people do want to compose refs, even if they don't know it (just thinking about my own experience but I may be wrong here).

Anyway I put up a preliminary version here https://github.com/ide/react-native-clone-referenced-element and am trying it out in a couple of components.

I'd like to see refs get less special/magical over time, not more special/magical

Yeah. I believe it's actually possible to kill refs almost entirely and replace them with a discrete version of @vjeux's Animated library. The Animated value would simulate a Kronecker delta and send an impulse to trigger an animation, give focus to a text field, etc... There's a whole other discussion there for another time =)

[sebmarkbage]

My concern with chaining refs implicitly is that you may introduce subtle bugs by having two sources touching an imperative API. You can't reason about how they interact. It can also lead to subtle race conditions. There's no single source of truth of the state - which is what React is all about.

I do agree that it is common for people to forget about the ref and that others might want access to it. However, I think that it is very important to make the choice explicitly. If they didn't, you can just ask them to do that explicitly or do it yourself. This is also one of the primary reasons we discourage React.findDOMNode for the benefit of direct refs since it allow you to by-pass the contract of a child and get access to a deeper ref.

Regarding replacing refs with data binding, I think that every possible use case in React could be replaced by data binding. We know because there are so many FRP libraries that have done it and we know how to do it. We also know that can lead to massive complexity. It is very difficult for us to make the call when we should move towards that direction since we know how it usually turns out. That's why I think that we should be very hesitant with data binding, but pragmatic when we need to be (e.g. Animated) - i.e. not as a general direction of the project.

refs is an espace hatch. It allows us access to solutions we haven't thought about. If we think about them, even just briefly, I think we can come up with great alternatives. For example, there are some ideas around focus and selection using parent-based context.

For higher order components specifically, it is usually the case that you want a pass-through ref. The current alternative is to have pass-through proxy methods on the higher order component. See #4213

It seems weird to use cloneElement with higher-order components. @ide can you elaborate on how you use it?

Finally, cloneElement is mostly used as a way to pass extra props from a parent to child. However, that doesn't work through wrappers. This is one of the primary features of parent-based context so we expect a lot of uses of cloneElement to be replaced by parent-based context anyway. The exception is when you need a ref ofc.

[ide]

@sebmarkbage thanks for the in-depth response. I am totally on board with controlling the complexity and not treating refs too magically. You convinced me with "two sources touching an imperative API" so I'm going to close this issue for now.

Regarding use cases, I compose refs with higher-order scroll views ex: https://github.com/exponentjs/react-native-infinite-scroll-view/blob/master/InfiniteScrollView.js#L40. I use pass-through proxy methods rather than passing InfiniteScrollView's ref down to ScrollView so that this works:

render() {
  return (
    <InfiniteScrollView
      ref={c => this._outer = c }
      renderScrollComponent={props =>
        <ScrollView {...props} ref={c => this._inner = c } />
      }
    />
  );
}

// Can refer to this._outer and this._inner separately as needed, and if there are methods defined only on
// InfiniteScrollView they are accessible via this._outer

This is a unique case because InfiniteScrollView, InvertibleScrollView, etc are not generic higher-order components, but rather conform to a standard scrollable component API. And the composition happens with the renderScrollComponent function rather than a class decorator or similar.

I am pretty happy with my cloneReferencedElement helper so I will keep using that.