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.

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-8041 Implement/Expose useImperativeHandle Hook #247

Merged
merged 18 commits into from
Jan 31, 2020

Conversation

sydneyjodon-wk
Copy link
Collaborator

@sydneyjodon-wk sydneyjodon-wk commented Jan 14, 2020

Motivation

Support useImperativeHandle hook in react-dart.

Changes

  • Added useImperativeHandle function.
  • Wrote tests.

Please review: @greglittlefield-wf @aaronlademann-wf @joebingham-wk

@sydneyjodon-wk sydneyjodon-wk changed the base branch from CPLAT-8040-useref-hook to 5.4.0-wip January 16, 2020 16:51
@sydneyjodon-wk sydneyjodon-wk marked this pull request as ready for review January 16, 2020 17:49
Copy link
Collaborator

@joebingham-wk joebingham-wk left a comment

Choose a reason for hiding this comment

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

This looks dooope! Refs are crazy and confusing, but the example is really straight forward and helped me figure out what was going on.

Most of my comments are just about nit picky typing stuff - all of the most important things looked awesome. Besides the comments, could we add an example using the dependencies parameter? I kinda get it, but it's still a little foggy and I think an example could help!

example/test/function_component_test.dart Outdated Show resolved Hide resolved
example/test/function_component_test.dart Outdated Show resolved Hide resolved
example/test/function_component_test.dart Outdated Show resolved Hide resolved
example/test/function_component_test.dart Outdated Show resolved Hide resolved
example/test/function_component_test.dart Outdated Show resolved Hide resolved
example/test/function_component_test.dart Outdated Show resolved Hide resolved
lib/hooks.dart Show resolved Hide resolved
lib/hooks.dart Outdated Show resolved Hide resolved
@@ -53,6 +53,7 @@ abstract class React {
external static Function useCallback(Function callback, List dependencies);
external static ReactContext useContext(ReactContext context);
external static JsRef useRef([dynamic initialValue]);
external static void useImperativeHandle(JsRef ref, Function() createHandle, [List dependencies]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on whether or not my typing information of hooks.useImperativeHandle is correct, this would be updated to match:

Suggested change
external static void useImperativeHandle(JsRef ref, Function() createHandle, [List dependencies]);
external static void useImperativeHandle(JsRef ref, void Function() createHandle, [List<dynamic> dependencies]);

test/hooks_test.dart Show resolved Hide resolved
Copy link
Collaborator

@joebingham-wk joebingham-wk left a comment

Choose a reason for hiding this comment

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

This looks great!! +1

Copy link
Collaborator

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple things around the docs/examples.

ref,
() {
print('FancyInput: useImperativeHandle re-assigns ref.current');
return {'focus': () => inputRef.current.focus()};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This matches the JS implementation by using a Map, but I think it might be helpful to also show a Dart object being used (perhaps in another example), to illustrate that you can use typed objects and not just Maps.

class FancyInputApi {
  final void Function() focus;
  FancyInputApi._(this.focus);
}

final FancyInput = react.forwardRef((props, ref) {
  final inputRef = useRef();

  useImperativeHandle(
    ref,
    () {
      print('FancyInput: useImperativeHandle re-assigns ref.current');
      return FancyInputApi(() => inputRef.current.focus());
    },

    /// Because the return value of createHandle never changes, it is not necessary for ref.current
    /// to be re-set on each render so this dependency list is empty.
    [],
  );
});

It is admittedly a bit clunkier than using a Map, but it does provide static typing.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense!

example/test/function_component_test.dart Outdated Show resolved Hide resolved
example/test/function_component_test.dart Outdated Show resolved Hide resolved
example/test/function_component_test.dart Outdated Show resolved Hide resolved
lib/hooks.dart Outdated Show resolved Hide resolved
sydneyjodon-wk and others added 3 commits January 23, 2020 15:33
…erativeHandle-hook

# Conflicts:
#	example/test/function_component_test.dart
#	lib/hooks.dart
#	lib/react_client/react_interop.dart
#	test/hooks_test.dart
Copy link
Collaborator

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+10

Just one #nit... but I pulled it down and tested it - looks good!

lib/hooks.dart Outdated Show resolved Hide resolved
@aaronlademann-wf aaronlademann-wf added this to the 5.4.0 milestone Jan 29, 2020
Copy link
Collaborator

@joebingham-wk joebingham-wk left a comment

Choose a reason for hiding this comment

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

+10

@joebingham-wk joebingham-wk merged commit 8b482c8 into 5.4.0-wip Jan 31, 2020
@greglittlefield-wf greglittlefield-wf deleted the CPLAT-8041-useImperativeHandle-hook branch February 16, 2022 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants