RFC #30: React.forwardRef implementation

[bvaughn]

Add new API, React.forwardRef, implemented as a new element type. Usage is e.g.:

const ThemeContext = React.createContext("light");

// Example HOC
function withTheme(ThemedComponent) {
  function ThemeContextInjector(props) {
    return (
      <ThemeContext.Consumer>
        {value => (
          <ThemedComponent {...props} ref={props.forwardedRef} theme={value} />
        )}
      </ThemeContext.Consumer>
    );
  }

  // Forward refs through to the inner, "themed" component:
  return React.forwardRef((props, ref) => (
    <ThemeContextInjector {...props} forwardedRef={ref} />
  ));
}

const ThemedButton = withTheme(Button);
const buttonRef = React.createRef();

// buttonRef.value will point to ThemedButton, rather than ThemeContextInjector
<ThemedButton ref={buttonRef} />;

Relates to reactjs/rfcs/pull/30

[TrySound]

"Render prop" in warning may confuse users. Maybe "render function"?

[bvaughn]

Sure, that's reasonable 👍

[jquense]

can I just say that It's really cool how async mode as inadvertently tipped the scales on all these long running TBD issues like context and refs :)

👏 Awesome work Brian! 🙇

[bvaughn]

Agreed!

Thanks, Jason. 😄

[sebmarkbage]

@jquense (That is true that it’s a forcing function but a little bit of the credit goes to us having a 3 times larger and ramped up amazing team now. ❤️)

[jquense]

❤️ ya go team! y'all are crushing it

[gaearon]

Wow why are these things so easy with Fiber

[bvaughn]

I know! 😁

[gaearon]

Might want to validate before calling to get a nicer error message. See a similar check before context. I think Seb told me once it doesn’t affect perf because engine has to check it’s a function regardless and will likely remember that.

[bvaughn]

I already validate in packages/react/src/useRef.js when the prop is passed initially.

[aweary]

The validation in useRef.js is just a warning though, maybe use an invariant and throw early instead?

[gaearon]

Hmm maybe we should change context to work the same way then.

I don’t think we should make it ever invariant during creation. Makes inlining harder and will likely cause duplicate checks.

[aweary]

Is the intention to rely on the warning to explain the error that would occur here when renderProp isn't a function? I understand not throwing during creation, but an explicit error seems better to me.

[bvaughn]

I'm not sure what consensus we've come to here. 😄

I think the warning during creation is good. Do we also want additional checks/invariants here? Or is it unnecessary?

[aweary]

I think warning during creation is a good idea, but I also think there should be an explicit error with a component stack trace when updateUseRef inevitably throws.

[bvaughn]

Is 166d236 what you had in mind? The component stack will only be included in DEV mode.

This feels a little weird to me, checking both places.

[sebmarkbage]

Warning > Invariant

[bvaughn]

I assume @sebmarkbage's comment is directed towards @aweary?

This PR is using warning 😄

[bvaughn]

This error message isn't great 😄 Any suggestions?

[aweary]

(moving this conversation out of the collapsed comment thread)

Is 166d236 what you had in mind? The component stack will only be included in DEV mode.
This feels a little weird to me, checking both places.

@bvaughn having a warning and invariant seems fine to me. There's precedence for it as well; you get a warning at creation time for invalid element types, and an error during render. Maybe the component stack could go in the warning in that case?

[bvaughn]

Maybe the component stack could go in the warning in that case?

There's no component stack for the warning, right? (useRef would typically be called from a factory function.)

[aweary]

Ah, right, that's true 🙂I don't think the component stack critical. If it can be included in the invariant, that's cool (errors get stripped in production anyway, right? so it wouldn't matter if it's DEV-only) but if not that's probably fine too.

[acdlite]

It occurs to me that we might be able to avoid a new Fiber type by storing the ref attribute as a non-enumerable symbol property. Then, useRef could look like this:

function useRef(props, ...rest) {
  const ref = props[REF_SYMBOL];  
  return component => {
    return component(props, ref, ...rest);
  };
}

It's possible I'm overlooking something. Also not sure if this is better or worse.

[bvaughn]

I dig how much simpler that suggestion would be to implement, but Sebastian has concerns about the perf of modifying createElement.

[bvaughn]

Renamed useRef to forwardRef

[sebmarkbage]

If we take a stab at implementing this in ReactChildFiber instead, we can make this a call-through instead of a Fiber. (We should do the same with stateless functional at some point.)

[sebmarkbage]

if (__DEV__) around it, right?

[bvaughn]

warning is an empty function if not DEV, so I didn't think that was necessary. But sure!

[sebmarkbage]

This is not really a render prop since it's not passed at render time nor a prop. It's more like, just renderFn or render maybe?

[bvaughn]

Good suggestion. I like renderFn