CPLAT-8034 Implement/Expose useState Hook #223

sydneyjodon-wk · 2019-10-30T18:08:45Z

Motivation

Support useState hook in react-dart

Changes

Added StateHook class
Added useState and useStateLazy functions
Wrote tests.

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

example/test/react_test_components.dart

lib/react_client/react_interop.dart

lib/hooks.dart

kealjones-wk

Just a few things :) excited for this, nice work

test/lifecycle_test.dart

lib/hooks.dart

example/test/react_test_components.dart

aaronlademann-wf · 2019-11-05T18:48:37Z

lib/hooks.dart

@@ -0,0 +1,97 @@
+@JS()
+library hooks;


Is there a reason we are making hooks a standalone public entrypoint? Any reason to not move this to lib/src/hooks.dart and then export that from lib/react.dart like we are doing for the context APIs?

After going back and forth on it, I suggested this offline to makes things a little more organized as opposed to just lumping more things into react.dart. Also, since they're APIs that will only be used sometimes, I figured it made sense for them to be their own self-contained entrypoint.

But, I could be convinced to move them into react.dart.

I feel the same it makes sense to have them seperate but i also enjoy the convenience of getting them when importing react.dart... plus they would get tree shaken out if they weren't used right?

lib/hooks.dart

aaronlademann-wf · 2019-11-05T18:52:19Z

lib/hooks.dart

+  /// Updates [value] to the return value of [computeNewValue].
+  ///
+  /// See: <https://reactjs.org/docs/hooks-reference.html#functional-updates>.
+  void setTx(T computeNewValue(T oldValue)) => _setValue(allowInterop(computeNewValue));


What does Tx represent? Does this naming convention map to an analogous pattern in the JS world?

It was mean to be "transactional", but it was clear when I had setStateTx that that wasn't clear haha...

Perhaps setWithUpdater would be better? Or even just update? But with the latter it might not be the immediately obvious the difference between set and update.

Thoughts? I think that for these kinds of naming decisions, we should gather input from more rather than fewer people. @joebingham-wk @kealjones-wk

i think setWithUpdater makes a lot of sense as it correlates to component2's new setStateWithUpdater, also we never made a codemod for that did we .... should we?

I like setWithUpdater

agreed, i also like, setWithUpdater

aaronlademann-wf · 2019-11-05T18:53:32Z

lib/hooks.dart

+  T _value;
+
+  /// The second item in the pair returned by [React.useState].
+  void Function(dynamic) _setValue;


I believe the argument here should be typed as T.

Suggested change

void Function(dynamic) _setValue;

void Function(T newValue) _setValue;

You might be right... my brain hurts when thinking about Dart 2 function types haha, so I'm not sure.

Works for me so long as there are no type errors in DDC or dart2js!

Changing this type breaks the tests for setTx. I think it's because _setValue is the updater function returned by the JS useState and can take an argument of type T newValue or type T computeNewValue(T oldValue).

https://github.com/dart-lang/sdk/issues/4938
dart-lang/language#83

lib/hooks.dart

aaronlademann-wf · 2019-11-05T19:03:01Z

lib/hooks.dart

+///       return react.div({}, [
+///         count.value,
+///         react.button({'onClick': (_) => count.set(0)}, ['Reset']),
+///         react.button({'onClick': (_) => count.setTx((prev) => prev + 1)}, ['+']),


Is this also possible without using setTx?

react.button({'onClick': (_) => count.set(count.value + 1)}, ['+']),

You're right, I just wrote it that way because that was the example of functional updates in the React docs:
https://reactjs.org/docs/hooks-reference.html#functional-updates
Should I change it to set and use a different example for setTx?

I think so, yeah.

But we could also just make a follow-up ticket to improve the examples.

Okay, I updated the example here

This is actually a case where you would need to use setTx.

Let's say that two setState calls were batched together:

react.button({'onClick': (_) { count.setTx((prev) => prev + 1); count.setTx((prev) => prev + 1); }, ['+2']),

Clicking on the button would result in the value being incremented by two.

However, without setTx:

react.button({'onClick': (_) { count.set(count.value + 1); count.set(count.value + 1); }, ['+2']),

Clicking on the button would result in the value being incremented by one, which is not expected.

Here, it would be best practice to use transactional setState calls.

React recommends using transactional state updaters where possible, and it also uses them in this corresponding example, so I think we should keep setTx here

lib/hooks.dart

# Conflicts: # example/test/react_test_components.dart

lib/hooks.dart

greglittlefield-wf

+10

greglittlefield-wf · 2019-11-13T17:24:25Z

example/test/function_component_test.dart

+    count.value,
+    react.button({'onClick': (_) => count.set(0)}, ['Reset']),
+    react.button({
+      'onClick': (_) => count.setWithUpdater((prev) {


#nit it's better to avoid the state change altogether when disabled:

'onClick': (_) { if (props['enabled']) { count.setWithUpdater((prev) => prev + 1); } },

greglittlefield-wf · 2019-11-13T17:26:32Z

lib/hooks.dart

+///     count.value,
+///     react.button({'onClick': (_) => count.set(0)}, ['Reset']),
+///     react.button({
+///       'onClick': (_) => count.setWithUpdater((prev) {


#nit We should probably show avoiding the state change altogether when disabled:

'onClick': (_) { if (props['enabled']) { count.setWithUpdater((prev) => prev + 1); } },

Or for this example, just do:

'onClick': (_) => count.setWithUpdater((prev) => prev + 1)

kealjones-wk · 2019-11-20T16:40:42Z

+10

greglittlefield-wf · 2019-11-21T20:20:00Z

+1

+10'd above

sydneyjodon-wk added 4 commits October 30, 2019 11:06

Implement useState

b6bd085

Implement useStateInit

0a78d12

Add useState example

9ee8ff8

Add doc comments

7c7d6b8

sydneyjodon-wk changed the base branch from 5.1.0-wip to CPLAT-4309-react-hooks October 30, 2019 20:57

sydneyjodon-wk added 3 commits October 30, 2019 14:52

Add tests

904044b

Update comments

7d0eb8d

Format

86a7205

aaronlademann-wf mentioned this pull request Nov 4, 2019

CPLAT-3874: Function Components #221

Merged

kealjones-wk reviewed Nov 4, 2019

View reviewed changes

example/test/react_test_components.dart Show resolved Hide resolved

kealjones-wk reviewed Nov 4, 2019

View reviewed changes

lib/react_client/react_interop.dart Show resolved Hide resolved

sydneyjodon-wk added 2 commits November 4, 2019 11:14

Move useState to hooks.dart file

abc3596

Move useState to hooks.dart file

f073b95

kealjones-wk reviewed Nov 4, 2019

View reviewed changes

lib/hooks.dart Outdated Show resolved Hide resolved

kealjones-wk previously requested changes Nov 4, 2019

View reviewed changes

aaronlademann-wf added this to the 5.1.0 milestone Nov 4, 2019

sydneyjodon-wk added 2 commits November 4, 2019 14:30

Move hooks tests to new file

0318115

Update doc comment

e0a36a2

aaronlademann-wf previously requested changes Nov 5, 2019

View reviewed changes

aaronlademann-wf modified the milestones: 5.1.0, 5.2.0 Nov 5, 2019

greglittlefield-wf reviewed Nov 6, 2019

View reviewed changes

lib/hooks.dart Outdated Show resolved Hide resolved

sydneyjodon-wk added 4 commits November 6, 2019 08:58

Add backticks for examples

eeb5c70

Update based on reviewer feedback

8bcef06

Merge branch 'CPLAT-4309-react-hooks' into CPLAT-8034-usestate-hook

ca69247

# Conflicts: # example/test/react_test_components.dart

Update setTx to setWithUpdater

0dd585d

joebingham-wk reviewed Nov 11, 2019

View reviewed changes

lib/hooks.dart Outdated Show resolved Hide resolved

sydneyjodon-wk added 2 commits November 11, 2019 09:44

Update doc comment

37d9629

Add better setWithUpdater example

d8da3d7

sydneyjodon-wk mentioned this pull request Nov 13, 2019

CPLAT-8035 Implement/Expose useEffect Hook #227

Merged

Merge branch 'CPLAT-4309-react-hooks' into CPLAT-8034-usestate-hook

1cd8b95

greglittlefield-wf approved these changes Nov 13, 2019

View reviewed changes

sydneyjodon-wk added 2 commits November 13, 2019 11:15

Merge branch 'CPLAT-4309-react-hooks' into CPLAT-8034-usestate-hook

2f30149

Merge branch 'CPLAT-4309-react-hooks' into CPLAT-8034-usestate-hook

ebfda89

sydneyjodon-wk mentioned this pull request Nov 18, 2019

CPLAT-8037 Implement/Expose useReducer Hook #232

Merged

Update based on Greg's feedback

5aa03e7

sydneyjodon-wk mentioned this pull request Nov 18, 2019

CPLAT-8038 Implement/Expose useCallback Hook #233

Merged

greglittlefield-wf changed the base branch from CPLAT-4309-react-hooks to 5.2.0-wip November 21, 2019 20:43

greglittlefield-wf merged commit 9c615f7 into 5.2.0-wip Nov 21, 2019

aaronlademann-wf mentioned this pull request Aug 7, 2020

Release react 5.5.0 #265

Merged

greglittlefield-wf deleted the CPLAT-8034-usestate-hook branch February 16, 2022 21:53

	void Function(dynamic) _setValue;
	void Function(T newValue) _setValue;

CPLAT-8034 Implement/Expose useState Hook #223

CPLAT-8034 Implement/Expose useState Hook #223

Uh oh!

Conversation

sydneyjodon-wk commented Oct 30, 2019 • edited by kealjones-wk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kealjones-wk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sydneyjodon-wk Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kealjones-wk commented Nov 20, 2019

Uh oh!

greglittlefield-wf commented Nov 21, 2019

Uh oh!

Uh oh!

sydneyjodon-wk commented Oct 30, 2019 •

edited by kealjones-wk

Loading

sydneyjodon-wk Nov 6, 2019 •

edited

Loading