Skip to content

Improve forwardRef documentation, add displayName argument #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 23 additions & 16 deletions lib/react_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import 'package:meta/meta.dart';

import "package:react/react.dart";
import 'package:react/react_client/js_interop_helpers.dart';
import 'package:react/react_client/private_utils.dart';
import 'package:react/react_client/react_interop.dart';
import "package:react/react_dom.dart";
import 'package:react/react_dom_server.dart';
Expand Down Expand Up @@ -349,9 +350,6 @@ List<String> _filterSkipMethods(List<String> methods) {
@JS('Object.keys')
external List<String> _objectKeys(Object object);

@JS('Object.defineProperty')
external void _defineProperty(dynamic object, String propertyName, JsMap descriptor);

@Deprecated('6.0.0')
InteropContextValue _jsifyContext(Map<String, dynamic> context) {
var interopContext = new InteropContextValue();
Expand Down Expand Up @@ -886,16 +884,13 @@ class ReactDartFunctionComponentFactoryProxy extends ReactComponentFactoryProxy
final JsFunctionComponent reactFunction;

ReactDartFunctionComponentFactoryProxy(DartFunctionComponent dartFunctionComponent, {String displayName})
: this.displayName = displayName ?? _getJsFunctionName(dartFunctionComponent),
: this.displayName = displayName ?? getJsFunctionName(dartFunctionComponent),
this.reactFunction = _wrapFunctionComponent(dartFunctionComponent,
displayName: displayName ?? _getJsFunctionName(dartFunctionComponent));
displayName: displayName ?? getJsFunctionName(dartFunctionComponent));

@override
JsFunctionComponent get type => reactFunction;

static String _getJsFunctionName(Function object) =>
getProperty(object, 'name') ?? getProperty(object, '\$static_name');

/// Creates a function component from the given [dartFunctionComponent] that can be used with React.
///
/// [displayName] Sets the component name for debugging purposes.
Expand All @@ -916,7 +911,7 @@ class ReactDartFunctionComponentFactoryProxy extends ReactComponentFactoryProxy
JsFunctionComponent interopFunction = allowInterop(jsFunctionComponent);
if (displayName != null) {
// This is a work-around to display the correct name in the React DevTools.
_defineProperty(interopFunction, 'name', jsify({'value': displayName}));
defineProperty(interopFunction, 'name', jsify({'value': displayName}));
}
// ignore: invalid_use_of_protected_member
setProperty(interopFunction, 'dartComponentVersion', ReactDartComponentVersion.component2);
Expand Down Expand Up @@ -995,14 +990,26 @@ _convertEventHandlers(Map args) {
args.forEach((propKey, value) {
var eventFactory = eventPropKeyToEventFactory[propKey];
if (eventFactory != null && value != null) {
// Apply allowInterop here so that the function we store in [_originalEventHandlers]
// is the same one we'll retrieve from the JS props.
var reactDartConvertedEventHandler = allowInterop((events.SyntheticEvent e, [_, __]) {
value(eventFactory(e));
});
// Don't attempt to convert functions that have already been converted, or functions
// that were passed in as JS props.
final handlerHasAlreadyBeenConverted = unconvertJsEventHandler(value) != null;
if (!handlerHasAlreadyBeenConverted && !(isRawJsFunctionFromProps[value] ?? false)) {
// Apply allowInterop here so that the function we store in [_originalEventHandlers]
// is the same one we'll retrieve from the JS props.
var reactDartConvertedEventHandler = allowInterop((e, [_, __]) {
// To support Dart code calling converted handlers,
// check for Dart events and pass them through directly.
// Otherwise, convert the JS events like normal.
if (e is SyntheticEvent) {
value(e);
} else {
value(eventFactory(e as events.SyntheticEvent));
}
});

args[propKey] = reactDartConvertedEventHandler;
_originalEventHandlers[reactDartConvertedEventHandler] = value;
args[propKey] = reactDartConvertedEventHandler;
_originalEventHandlers[reactDartConvertedEventHandler] = value;
}
}
});
}
Expand Down
5 changes: 5 additions & 0 deletions lib/react_client/js_interop_helpers.dart
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,8 @@ _convertDataTree(data) {

return _convert(data);
}

/// Keeps track of functions found when converting JS props to Dart props.
///
/// See: [forwardRef] for usage / context.
final isRawJsFunctionFromProps = Expando<bool>();
12 changes: 12 additions & 0 deletions lib/react_client/private_utils.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
@JS()
library react_client.private_utils;

import 'dart:js_util';

import 'package:js/js.dart';
import 'js_backed_map.dart';

@JS('Object.defineProperty')
external void defineProperty(dynamic object, String propertyName, JsMap descriptor);

String getJsFunctionName(Function object) => getProperty(object, 'name') ?? getProperty(object, '\$static_name');
167 changes: 163 additions & 4 deletions lib/react_client/react_interop.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import 'package:react/react.dart';
import 'package:react/react_client.dart' show ComponentFactory, ReactJsComponentFactoryProxy;
import 'package:react/react_client/bridge.dart';
import 'package:react/react_client/js_backed_map.dart';
import 'package:react/react_client/js_interop_helpers.dart';
import 'package:react/react_client/private_utils.dart';
import 'package:react/src/react_client/dart2_interop_workaround_bindings.dart';

typedef ReactElement ReactJsComponentFactory(props, children);
Expand All @@ -31,6 +33,7 @@ typedef dynamic JsPropValidator(
@JS()
abstract class React {
external static String get version;
external static ReactElement cloneElement(ReactElement element, [JsMap props, dynamic children]);
external static ReactContext createContext([
dynamic defaultValue,
int Function(dynamic currentValue, dynamic nextValue) calculateChangedBits,
Expand Down Expand Up @@ -127,15 +130,171 @@ class JsRef {

/// Automatically passes a [Ref] through a component to one of its children.
///
/// __Not recommended for use with interop'd JS components. Use [forwardRefToJs] instead.__
///
/// __Example 1:__ Forwarding refs to DOM components
///
/// _[Analogous JS Demo](https://reactjs.org/docs/forwarding-refs.html#forwarding-refs-to-dom-components)_
///
/// ```dart
/// import 'dart:html';
/// import 'package:react/react.dart' as react;
///
/// // ---------- Component Definition ----------
/// final FancyButton = react.forwardRef((props, ref) {
/// return react.button({'ref': ref, 'className': 'FancyButton'}, 'Click me!');
/// }, displayName: 'FancyButton');
///
/// // ---------- Component Consumption ----------
/// void main() {
/// final ref = createRef<Element>();
///
/// react_dom.render(FancyButton({'ref': ref}));
///
/// // You can now get a ref directly to the DOM button:
/// final buttonNode = ref.current;
/// }
/// ```
///
/// __Example 2:__ Forwarding refs in higher-order components
///
/// _[Analogous JS Demo](https://reactjs.org/docs/forwarding-refs.html#forwarding-refs-in-higher-order-components)_
///
/// ```dart
/// import 'dart:html';
/// import 'package:react/react.dart' as react;
/// import 'package:react/react_client.dart' show setClientConfiguration, ReactJsComponentFactoryProxy;
/// import 'package:react/react_dom.dart' as react_dom;
///
/// // ---------- Component Definitions ----------
///
/// final FancyButton = react.forwardRef((props, ref) {
/// return react.button({'ref': ref, 'className': 'FancyButton'}, 'Click me!');
/// }, displayName: 'FancyButton');
///
/// class _LogProps extends react.Component2 {
/// @override
/// void componentDidUpdate(Map prevProps, _, [__]) {
/// print('old props: $prevProps');
/// print('new props: ${this.props}');
/// }
///
/// @override
/// render() {
/// final propsToForward = {...props}..remove('forwardedRef');
///
/// // Assign the custom prop `forwardedRef` as a ref on the component passed in via `props.component`
/// return props['component']({...propsToForward, 'ref': props['forwardedRef']}, props['children']);
/// }
/// }
/// final _logPropsHoc = react.registerComponent2(() => _LogProps());
///
/// final LogProps = react.forwardRef((props, ref) {
/// // 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 _logPropsHoc({...props, 'forwardedRef': ref});
/// // Optional: Make the displayName more useful for the React dev tools.
/// // See: https://reactjs.org/docs/forwarding-refs.html#displaying-a-custom-name-in-devtools
/// }, displayName: 'LogProps(${_logPropsHoc.type.displayName})');
///
/// // ---------- Component Consumption ----------
/// void main() {
/// setClientConfiguration();
/// final ref = react.createRef<Element>();
///
/// react_dom.render(LogProps({'component': FancyButton, 'ref': ref}),
/// querySelector('#idOfSomeNodeInTheDom'));
///
/// // You can still get a ref directly to the DOM button:
/// final buttonNode = ref.current;
/// }
/// ```
/// See: <https://reactjs.org/docs/forwarding-refs.html>.
ReactJsComponentFactoryProxy forwardRef(Function(Map props, Ref ref) wrapperFunction) {
var hoc = React.forwardRef(allowInterop((JsMap props, JsRef ref) {
ReactJsComponentFactoryProxy forwardRef(
Function(Map props, Ref ref) wrapperFunction, {
String displayName = 'Anonymous',
}) {
final wrappedComponent = allowInterop((JsMap props, JsRef ref) {
final dartProps = JsBackedMap.backedBy(props);
for (var value in dartProps.values) {
if (value is Function) {
// Tag functions that came straight from the JS
// so that we know to pass them through as-is during prop conversion.
isRawJsFunctionFromProps[value] = true;
}
}

final dartRef = Ref.fromJs(ref);
return wrapperFunction(dartProps, dartRef);
}));
});
defineProperty(wrappedComponent, 'displayName', jsify({'value': displayName}));

var hoc = React.forwardRef(wrappedComponent);

return ReactJsComponentFactoryProxy(hoc);
}

/// Shorthand convenience function to wrap a JS component that utilizes `forwardRef`.
///
/// __Example:__
///
/// ```dart
/// // Assuming that your app includes the JS file containing the MaterialUI components...
/// @JS()
/// class MaterialUI {
/// external static ReactClass get TextField;
/// }
///
/// /// This makes it so that the `inputRef` set on the Dart `TextField` component
/// /// gets passed all the way to the root <input> node rendered by the JS `MaterialUI.TextField` component.
/// final TextField = forwardRefToJsComponent(MaterialUI.IconButton, additionalRefPropKeys: ['inputRef']);
///
/// void main() {
/// setClientConfiguration();
/// final textFieldInputNodeRef = react.createRef<TextInputElement>();
///
/// react_dom.render(TextInput({'inputRef': textFieldInputNodeRef}), querySelector('#idOfSomeNodeInTheDom'));
///
/// // Prints the value of the <input> rendered by the JS `MaterialUI.TextField` component.
/// print(textFieldInputNodeRef.current.value);
/// }
/// ```
ReactJsComponentFactoryProxy forwardRefToJs(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ReactClass jsClassComponent, {
List<String> additionalRefPropKeys = const [],
String displayName,
}) {
// Do not convert DOM props so that the events passed into the JS component are NOT Dart `SyntheticEvent`s.
final jsFactoryProxy = ReactJsComponentFactoryProxy(jsClassComponent);

return forwardRef((props, ref) {
return jsFactoryProxy({
..._convertNonDefaultRefPropKeysToJs(props, additionalRefPropKeys),
'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}');
}

Map _convertNonDefaultRefPropKeysToJs(Map props, List<String> additionalRefPropKeys) {
var propsMapToReturn = props;
if (additionalRefPropKeys.isNotEmpty) {
var propsWithConvertedRefs = {...props};
for (var propKey in additionalRefPropKeys) {
var ref = propsWithConvertedRefs[propKey];

// The default 'ref' prop key value gets converted by default
// by ReactJsComponentFactoryProxy, so we don't need to do it here.
if (propKey != 'ref' && ref is Ref) {
propsWithConvertedRefs[propKey] = ref.jsRef;
}
}

propsMapToReturn = propsWithConvertedRefs;
}

return new ReactJsComponentFactoryProxy(hoc, shouldConvertDomProps: false);
return propsMapToReturn;
}

abstract class ReactDom {
Expand Down
Loading