Improve forwardRef documentation, add displayName argument

[aaronlademann-wf]

While grokking our forwardRef API, I found myself wanting some implementation examples which could not be found in the documentation comment.

I also noticed that consumers had no way to define a displayName for the wrapper HOC created by forwardRef.

These changes address both issues by adding extensive examples in the documentation comment for forwardRef that mirror the analogous ReactJS forwardRef documentation, and by adding a displayName argument to the function which then gets set on the JS side via a call to defineProperty.

---

FYA: @greglittlefield-wf @joebingham-wk @sydneyjodon-wk

[joebingham-wk]

This looks awesome! Just a thought and nit, but them I'm +1!

[greglittlefield-wf]

Taking a fresh look at what this functionality provides, I think this name is misleading, since it doesn't really provide similar functionality to forwardRef.

What about something like withJsRefsConverted? Also, additionalRefPropKeys should probably be renamed to not have additional, since this function doesn't really do anything without them

[aaronlademann-wf]

I'm confused what you mean by "doesn't really provide similar functionality to forwardRef".

This function returns the return value of a call to forwardRef.

[greglittlefield-wf]

Right, but it doesn't allow you to forward a ref onto a different component/prop. It's really just a HOC that's using forwardRef.

For example, the last snippet in this section: https://reactjs.org/docs/forwarding-refs.html#forwarding-refs-in-higher-order-components

function logProps(Component) {
  class LogProps extends React.Component {
    componentDidUpdate(prevProps) {
      console.log('old props:', prevProps);
      console.log('new props:', this.props);
    }

    render() {
      const {forwardedRef, ...rest} = this.props;

      // Assign the custom prop "forwardedRef" as a ref
      return <Component ref={forwardedRef} {...rest} />;
    }
  }

  // Note the second param "ref" provided by React.forwardRef.
  // We can pass it along to LogProps as a regular prop, e.g. "forwardedRef"
  // And it can then be attached to the Component.
  return React.forwardRef((props, ref) => {
    return <LogProps {...props} forwardedRef={ref} />;
  });
}

logProps isn't logPropsForwardRef, it just uses it so that the HOC passes the ref through.

[aaronlademann-wf]

@greglittlefield-wf I disagree that from an API clarity standpoint - withJsRefsConverted is more clear about the purpose of the function.

The purpose of the function is so that refs get forwarded. All the HOC wrapping is semantics, IMO. The function does more than just convert refs... it also forwards them to the JS component.

[greglittlefield-wf]

Okay, so taking a step back...

At this point, this function is just a wrapper around ReactJsComponentFactoryProxy, and the forwardRef call really serves no purpose.

With the following changes:

-  return forwardRef((props, ref) {
-    return jsFactoryProxy({
-      ...props,
-      'ref': ref,
-    }, props['children']);
-    // Convert DOM props for the HOC so the consumer API callbacks that involve events receive Dart `SyntheticEvent`s.
-  }, displayName: '${displayName ?? jsClassComponent.displayName}');
+  return jsFactoryProxy;

The behavior would be exactly the same.

So, why do we need this function at all, if it's just a wrapper around ReactJsComponentFactoryProxy? Consumers should just use that instead.

[joebingham-wk]

A few more nits! The var vs final are probably actually #supernits, but the formatting of the if statements is probably worth cleaning up since it does make it harder to read

[joebingham-wk]

#nit ref could be final

Suggested change

	var ref = args[refKey];
	final ref = args[refKey];

[joebingham-wk]

+1

[greglittlefield-wf]

+10