Skip to content
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.

Sign up for GitHub

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

CPLAT-11977 Consume/update typing for forwardRef2/memo2 functions #620

Merged
merged 5 commits into from
Sep 10, 2020
Merged

CPLAT-11977 Consume/update typing for forwardRef2/memo2 functions #620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e368225
Consume/update typing for forwardRef2/memo2 functions
aaronlademann-wf Aug 13, 2020
1fb6edd
Fix uiForwardRef name behavior and tests
greglittlefield-wf Sep 9, 2020
871c9c7
Fix useImperativeHandle typing and example
greglittlefield-wf Sep 9, 2020
73beab1
Simplify useImperativeHandle example
greglittlefield-wf Sep 9, 2020
85c2690
Point to released react-dart version with forwardRef2/memo2
greglittlefield-wf Sep 10, 2020
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
18 changes: 4 additions & 14 deletions example/hooks/use_imperative_handle_example.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ part 'use_imperative_handle_example.over_react.g.dart';
mixin FancyInputProps on UiProps {
String value;
Function updater;
Ref forwardedRef;
}

/// The type of ref returned when rendering [UseImperativeHandleExample].
Expand All @@ -31,21 +30,12 @@ class FancyInputApi {
FancyInputApi(this.focus);
}

UiFactory<FancyInputProps> FancyInput = uiForwardRef((props, ref) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified this while i was in here fixing the typing on forwardedRef

return (_FancyInput()
..forwardedRef = ref
..addProps(props)
)();
},
_FancyInput.asForwardRefConfig(displayName: 'FancyInputForwardRef')
);

UiFactory<FancyInputProps> _FancyInput = uiFunction(
(props) {
UiFactory<FancyInputProps> FancyInput = uiForwardRef(
(props, ref) {
final inputRef = useRef<InputElement>();

useImperativeHandle(
props.forwardedRef,
ref,
() => FancyInputApi(() => inputRef.current.focus()),

/// Because the return value of [createHandle] never changes, it is not necessary for [ref.current]
Expand All @@ -59,7 +49,7 @@ UiFactory<FancyInputProps> _FancyInput = uiFunction(
..onChange = (e) => props.updater(e.target.value)
)();
},
$_FancyInputConfig, // ignore: undefined_identifier
$FancyInputConfig, // ignore: undefined_identifier
);

mixin UseImperativeHandleExampleProps on UiProps {}
Expand Down
21 changes: 4 additions & 17 deletions example/hooks/use_imperative_handle_example.over_react.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions lib/src/component/hooks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ void useLayoutEffect(dynamic Function() sideEffect, [List<Object> dependencies])
/// ```
///
/// Learn more: <https://reactjs.org/docs/hooks-reference.html#useimperativehandle>.
void useImperativeHandle(Ref ref, dynamic Function() createHandle, [List<dynamic> dependencies]) =>
void useImperativeHandle(dynamic ref, dynamic Function() createHandle, [List<dynamic> dependencies]) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has just been updated to match the typing of the react-dart version

react_hooks.useImperativeHandle(ref, createHandle, dependencies);

/// Displays [value] as a label for a custom hook in React DevTools.
Expand Down Expand Up @@ -544,4 +544,4 @@ void useImperativeHandle(Ref ref, dynamic Function() createHandle, [List<dynamic
/// ```
///
/// Learn more: <https://reactjs.org/docs/hooks-reference.html#usedebugvalue>.
dynamic useDebugValue<T>(T value, [dynamic Function(T) format]) => react_hooks.useDebugValue(value, format);
dynamic useDebugValue<T>(T value, [dynamic Function(T) format]) => react_hooks.useDebugValue(value, format);
13 changes: 4 additions & 9 deletions lib/src/component/ref_util.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,15 +412,10 @@ UiFactory<TProps> uiForwardRef<TProps extends bh.UiProps>(
return functionComponent(propsFactory.jsMap(props), ref);
}

ReactJsComponentFactoryProxy factory;

// If a consumer uses `asForwardRefConfig` to generate the function component
// config, displayName could be `null` or an empty string.
if (displayName != null && displayName.isNotEmpty) {
factory = react_interop.forwardRef(_uiFunctionWrapper, displayName: displayName);
} else {
factory = react_interop.forwardRef(_uiFunctionWrapper);
}
// Always pass displayName, even if it's empty,
// since we don't want forwardRef2 to use _uiFunctionWrapper as the name.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under the hood forwardRef2 falls back to the name of the function if displayName is null, as opposed to the old default of Anonymous.

If the name is empty, it will still show up as "Anonymous" in the dev tools, so functionality is equivalent.

final factory =
react_interop.forwardRef2(_uiFunctionWrapper, displayName: displayName);

if (propsFactory == null) {
if (TProps != UiProps && TProps != GenericUiProps) {
Expand Down
3 changes: 2 additions & 1 deletion lib/src/component_declaration/component_type_checking.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import 'package:over_react/src/component_declaration/annotations.dart'
import 'package:over_react/src/util/react_wrappers.dart';
import 'package:over_react/src/util/string_util.dart';
import 'package:react/react_client.dart';
import 'package:react/react_client/component_factory.dart';
import 'package:react/react_client/react_interop.dart';

// ----------------------------------------------------------------------
Expand Down Expand Up @@ -301,7 +302,7 @@ void enforceMinimumComponentVersionFor(ReactComponentFactoryProxy component) {

/// Validates that a [ReactComponentFactoryProxy]'s component is a function component.
void enforceFunctionComponent(ReactComponentFactoryProxy component) {
if (component is ReactDartFunctionComponentFactoryProxy) return;
if (component is ReactDartFunctionComponentFactoryProxy || component is ReactDartWrappedComponentFactoryProxy) return;

throw ArgumentError(unindent('''
The UiFactory provided should be for a function component.
Expand Down
4 changes: 2 additions & 2 deletions lib/src/util/memo.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ UiFactory<TProps> memo<TProps extends UiProps>(UiFactory<TProps> factory,
return areEqual(tPrevProps, tNextProps);
}

hoc = react_interop.memo(factory().componentFactory, areEqual: wrapProps);
hoc = react_interop.memo2(factory().componentFactory, areEqual: wrapProps);
} else {
hoc = react_interop.memo(factory().componentFactory);
hoc = react_interop.memo2(factory().componentFactory);
}

setComponentTypeMeta(hoc,
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ dependencies:
memoize: ^2.0.0
meta: ^1.1.6
path: ^1.5.1
react: ^5.5.0
react: ^5.6.0
redux: ">=3.0.0 <5.0.0"
source_span: ^1.4.1
transformer_utils: ^0.2.0
Expand Down
35 changes: 25 additions & 10 deletions test/over_react/component/ref_util_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ main() {
final Ref refObject = createRef();
final vDomElement = (BasicForwarded()..ref = refObject)();

expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'), 'Anonymous');
expect(getProperty(getProperty(vDomElement.type, 'render'), 'name'), '');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. forwardRef2 uses name, not displayName
  2. An empty string is used instead of Anonymous in forwardRef2 (the empty string shows up as "Anonymous" in the dev tools, so functionality is equivalent), so these needed to be updated

});

test('when displayName argument is passed to the config constructor', () {
Expand All @@ -71,7 +71,7 @@ main() {
final Ref refObject = createRef();
final vDomElement = (BasicForwarded()..ref = refObject)();

expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'), displayName);
expect(getProperty(getProperty(vDomElement.type, 'render'), 'name'), displayName);
});

group('returns normally when passed children', () {
Expand Down Expand Up @@ -99,7 +99,7 @@ main() {
final Ref refObject = createRef();
final vDomElement = (TopLevelForwardUiRefFunction()..ref = refObject)();

expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'),
expect(getProperty(getProperty(vDomElement.type, 'render'), 'name'),
'TopLevelForwardUiRefFunction');
});

Expand Down Expand Up @@ -153,7 +153,7 @@ void commonRefForwardingTests({bool useUiForwardRef = false}) {
UiFactory<DomProps> getFactoryForDiv({
String displayName,
}) {
ReactElement div(Ref ref, dynamic children) => (Dom.div()..ref = ref)(children);
ReactElement div(dynamic ref, dynamic children) => (Dom.div()..ref = ref)(children);

if (useUiForwardRef) {
if (displayName == null) {
Expand Down Expand Up @@ -201,8 +201,11 @@ void commonRefForwardingTests({bool useUiForwardRef = false}) {
final refObject = createRef<DivElement>();
final vDomElement = (DivForwarded()..ref = refObject)();

expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'),
useUiForwardRef ? 'Anonymous' : 'div');
if (useUiForwardRef) {
expect(getProperty(getProperty(vDomElement.type, 'render'), 'name'), '');
} else {
expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'), 'div');
}
});

test('when displayName argument is passed to the config constructor', () {
Expand All @@ -211,7 +214,12 @@ void commonRefForwardingTests({bool useUiForwardRef = false}) {
final refObject = createRef<DivElement>();
final vDomElement = (DivForwarded()..ref = refObject)();

expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'), displayName);

if (useUiForwardRef) {
expect(getProperty(getProperty(vDomElement.type, 'render'), 'name'), displayName);
} else {
expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'), displayName);
}
});
});

Expand All @@ -238,8 +246,11 @@ void commonRefForwardingTests({bool useUiForwardRef = false}) {
final Ref refObject = createRef();
final vDomElement = (BasicForwarded()..ref = refObject)();

expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'),
useUiForwardRef ? 'Anonymous' : 'Basic');
if (useUiForwardRef) {
expect(getProperty(getProperty(vDomElement.type, 'render'), 'name'), '');
} else {
expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'), 'Basic');
}
});

test('when displayName argument is passed to the config constructor', () {
Expand All @@ -248,7 +259,11 @@ void commonRefForwardingTests({bool useUiForwardRef = false}) {
final Ref refObject = createRef();
final vDomElement = (BasicForwarded()..ref = refObject)();

expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'), displayName);
if (useUiForwardRef) {
expect(getProperty(getProperty(vDomElement.type, 'render'), 'name'), displayName);
} else {
expect(getProperty(getProperty(vDomElement.type, 'render'), 'displayName'), displayName);
}
});
});

Expand Down