Skip to content

CPLAT-8035 Implement/Expose useEffect Hook #227

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 33 commits into from
Jan 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
8bde18c
Add useEffect hook and update example
sydneyjodon-wk Nov 4, 2019
44255e9
Update example and add dependencies arg
sydneyjodon-wk Nov 4, 2019
d9f5163
Merge branch 'CPLAT-8034-usestate-hook' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Nov 6, 2019
9bd0bf3
merge
sydneyjodon-wk Nov 6, 2019
ca578f5
format
sydneyjodon-wk Nov 6, 2019
30a81b1
Add tests and doc comments
sydneyjodon-wk Nov 11, 2019
6211f98
Format
sydneyjodon-wk Nov 11, 2019
dd78b2c
Update tests
sydneyjodon-wk Nov 11, 2019
eac37cd
Merge branch 'CPLAT-8034-usestate-hook' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Nov 11, 2019
745d07a
Format
sydneyjodon-wk Nov 11, 2019
e3fe834
Wrap return value in allowInterop
sydneyjodon-wk Nov 11, 2019
de5292e
Update tests
sydneyjodon-wk Nov 11, 2019
87b912f
Format
sydneyjodon-wk Nov 11, 2019
2435c30
Update tests
sydneyjodon-wk Nov 11, 2019
4721778
Format
sydneyjodon-wk Nov 13, 2019
d0af4d2
Add additional testcases
sydneyjodon-wk Nov 13, 2019
880c7d3
Update naming
sydneyjodon-wk Nov 13, 2019
d28648b
Update tests
sydneyjodon-wk Nov 13, 2019
88cddec
Merge branch 'CPLAT-4309-react-hooks' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Nov 13, 2019
29b128f
Merge branch 'CPLAT-8034-usestate-hook' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Nov 13, 2019
d30390b
Merge branch 'CPLAT-8034-usestate-hook' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Nov 13, 2019
5c76190
Merge branch 'CPLAT-8034-usestate-hook' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Nov 13, 2019
716238b
Merge branch 'CPLAT-8034-usestate-hook' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Nov 18, 2019
368f13e
Update based on reviewer feedback
sydneyjodon-wk Nov 20, 2019
1503565
Merge branch '5.2.0-wip' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Nov 25, 2019
2798ad1
Add doc comment to example
sydneyjodon-wk Nov 25, 2019
8668f9e
Format
sydneyjodon-wk Nov 25, 2019
f8a7fe3
Change side effect to return `undefined` instead of `null`
sydneyjodon-wk Dec 2, 2019
0c6b313
Update based on reviewer feedback
sydneyjodon-wk Dec 2, 2019
e5ed461
Merge branch '5.2.0-wip' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Dec 2, 2019
95142fd
Merge branch '5.3.0-wip' into CPLAT-8035-useeffect-hook
sydneyjodon-wk Jan 6, 2020
55f8151
Format
sydneyjodon-wk Jan 6, 2020
e234b38
Update based on reviewer feedback
sydneyjodon-wk Jan 6, 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
33 changes: 25 additions & 8 deletions example/test/function_component_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,32 @@ import 'package:react/react.dart' as react;
import 'package:react/react_dom.dart' as react_dom;
import 'package:react/react_client.dart';

var useStateTestFunctionComponent = react.registerFunctionComponent(UseStateTestComponent, displayName: 'useStateTest');

UseStateTestComponent(Map props) {
final count = useState(0);
var hookTestFunctionComponent = react.registerFunctionComponent(HookTestComponent, displayName: 'useStateTest');

HookTestComponent(Map props) {
final count = useState(1);
final evenOdd = useState('even');

useEffect(() {
if (count.value % 2 == 0) {
print('count changed to ' + count.value.toString());
evenOdd.set('even');
} else {
print('count changed to ' + count.value.toString());
evenOdd.set('odd');
}
return () {
print('count is changing... do some cleanup if you need to');
};

/// This dependency prevents the effect from running every time [evenOdd.value] changes.
}, [count.value]);

return react.div({}, [
count.value,
react.button({'onClick': (_) => count.set(0), 'key': 'ust1'}, ['Reset']),
react.button({'onClick': (_) => count.set(1), 'key': 'ust1'}, ['Reset']),
react.button({'onClick': (_) => count.setWithUpdater((prev) => prev + 1), 'key': 'ust2'}, ['+']),
react.br({'key': 'ust3'}),
react.p({'key': 'ust4'}, ['${count.value} is ${evenOdd.value}']),
]);
}

Expand Down Expand Up @@ -107,8 +124,8 @@ void main() {
'key': 'fctf'
}, [
react.h1({'key': 'functionComponentTestLabel'}, ['Function Component Tests']),
react.h2({'key': 'useStateTestLabel'}, ['useState Hook Test']),
useStateTestFunctionComponent({
react.h2({'key': 'useStateTestLabel'}, ['useState & useEffect Hook Test']),
hookTestFunctionComponent({
'key': 'useStateTest',
}, []),
react.br({'key': 'br'}),
Expand Down
56 changes: 56 additions & 0 deletions lib/hooks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,62 @@ StateHook<T> useState<T>(T initialValue) => StateHook(initialValue);
/// Learn more: <https://reactjs.org/docs/hooks-reference.html#lazy-initial-state>.
StateHook<T> useStateLazy<T>(T init()) => StateHook.lazy(init);

/// Runs [sideEffect] after every completed render of a [DartFunctionComponent].
///
/// If [dependencies] are given, [sideEffect] will only run if one of the [dependencies] have changed.
/// [sideEffect] may return a cleanup function that is run before the component unmounts or re-renders.
///
/// > __Note:__ there are two [rules for using Hooks](https://reactjs.org/docs/hooks-rules.html):
/// >
/// > * Only call Hooks at the top level.
/// > * Only call Hooks from inside a [DartFunctionComponent].
///
/// __Example__:
///
/// ```
/// UseEffectTestComponent(Map props) {
/// final count = useState(1);
/// final evenOdd = useState('even');
///
/// useEffect(() {
/// if (count.value % 2 == 0) {
/// evenOdd.set('even');
/// } else {
/// evenOdd.set('odd');
/// }
/// return () {
/// print('count is changing... do some cleanup if you need to');
/// };
///
/// // This dependency prevents the effect from running every time [evenOdd.value] changes.
/// }, [count.value]);
///
/// return react.div({}, [
/// react.p({}, ['${count.value} is ${evenOdd.value}']),
/// react.button({'onClick': (_) => count.set(count.value + 1)}, ['+']),
/// ]);
/// }
/// ```
///
/// See: <https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects>.
void useEffect(dynamic Function() sideEffect, [List<Object> dependencies]) {
var wrappedSideEffect = allowInterop(() {
var result = sideEffect();
if (result is Function) {
return allowInterop(result);
}

/// When no cleanup function is returned, [sideEffect] returns undefined.
return jsUndefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add a comment saying why undefined is needed?

});

if (dependencies != null) {
return React.useEffect(wrappedSideEffect, dependencies);
Comment on lines +154 to +155
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it ok / reasonably efficient to pass in an empty list here?

If not, we should probably check .isNotEmpty in addition to != null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, passing an empty list is used if you just want to run an effect and clean it up only once (on mount and unmount)

} else {
return React.useEffect(wrappedSideEffect);
}
}

/// Returns a memoized version of [callback] that only changes if one of the [dependencies] has changed.
///
/// > __Note:__ there are two [rules for using Hooks](https://reactjs.org/docs/hooks-rules.html):
Expand Down
6 changes: 6 additions & 0 deletions lib/react_client/react_interop.dart
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ abstract class React {
external static ReactClass forwardRef(Function(JsMap props, JsRef ref) wrapperFunction);

external static List<dynamic> useState(dynamic value);
external static void useEffect(dynamic Function() sideEffect, [List<Object> dependencies]);
external static Function useCallback(Function callback, List dependencies);
external static ReactContext useContext(ReactContext context);
}
Expand Down Expand Up @@ -488,6 +489,11 @@ class JsError {
@JS('_jsNull')
external get jsNull;

/// A JS variable that can be used with Dart interop in order to force returning a JavaScript `undefined`.
/// Use this if dart2js is possibly converting Dart `undefined` into `null`.
@JS('_jsUndefined')
external get jsUndefined;

/// Throws the error passed to it from Javascript.
/// This allows us to catch the error in dart which re-dartifies the js errors/exceptions.
@alwaysThrows
Expand Down
164 changes: 156 additions & 8 deletions test/hooks_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,6 @@ main() {
react_dom.render(UseStateTest({}), mountNode);
});

tearDownAll(() {
UseStateTest = null;
});

test('initializes state correctly', () {
expect(countRef.text, '0');
});
Expand All @@ -94,6 +90,162 @@ main() {
});
});

group('useEffect -', () {
ReactDartFunctionComponentFactoryProxy UseEffectTest;
ButtonElement countButtonRef;
DivElement countRef;
DivElement mountNode;
int useEffectCallCount;
int useEffectCleanupCallCount;
int useEffectWithDepsCallCount;
int useEffectCleanupWithDepsCallCount;
int useEffectWithDepsCallCount2;
int useEffectCleanupWithDepsCallCount2;
int useEffectWithEmptyDepsCallCount;
int useEffectCleanupWithEmptyDepsCallCount;

setUpAll(() {
mountNode = new DivElement();
useEffectCallCount = 0;
useEffectCleanupCallCount = 0;
useEffectWithDepsCallCount = 0;
useEffectCleanupWithDepsCallCount = 0;
useEffectWithDepsCallCount2 = 0;
useEffectCleanupWithDepsCallCount2 = 0;
useEffectWithEmptyDepsCallCount = 0;
useEffectCleanupWithEmptyDepsCallCount = 0;

UseEffectTest = react.registerFunctionComponent((Map props) {
final count = useState(0);
final countDown = useState(0);

useEffect(() {
useEffectCallCount++;
return () {
useEffectCleanupCallCount++;
};
});

useEffect(() {
useEffectWithDepsCallCount++;
return () {
useEffectCleanupWithDepsCallCount++;
};
}, [count.value]);

useEffect(() {
useEffectWithDepsCallCount2++;
return () {
useEffectCleanupWithDepsCallCount2++;
};
}, [countDown.value]);

useEffect(() {
useEffectWithEmptyDepsCallCount++;
return () {
useEffectCleanupWithEmptyDepsCallCount++;
};
}, []);

return react.div({}, [
react.div({
'ref': (ref) {
countRef = ref;
},
}, [
count.value
]),
react.button({
'onClick': (_) {
count.set(count.value + 1);
},
'ref': (ref) {
countButtonRef = ref;
},
}, [
'+'
]),
]);
});

react_dom.render(UseEffectTest({}), mountNode);
});

test('side effect (no dependency list) is called after the first render', () {
expect(countRef.text, '0');

expect(useEffectCallCount, 1);
expect(useEffectCleanupCallCount, 0, reason: 'component has not been unmounted or re-rendered');
});

test('side effect (with dependency list) is called after the first render', () {
expect(useEffectWithDepsCallCount, 1);
expect(useEffectCleanupWithDepsCallCount, 0, reason: 'component has not been unmounted or re-rendered');

expect(useEffectWithDepsCallCount2, 1);
expect(useEffectCleanupWithDepsCallCount2, 0, reason: 'component has not been unmounted or re-rendered');
});

test('side effect (with empty dependency list) is called after the first render', () {
expect(useEffectWithEmptyDepsCallCount, 1);
expect(useEffectCleanupWithEmptyDepsCallCount, 0, reason: 'component has not been unmounted or re-rendered');
});

group('after state change,', () {
setUpAll(() {
react_test_utils.Simulate.click(countButtonRef);
});

test('side effect (no dependency list) is called again', () {
expect(countRef.text, '1');

expect(useEffectCallCount, 2);
expect(useEffectCleanupCallCount, 1, reason: 'cleanup called before re-render');
});

test('side effect (with dependency list) is called again if one of its dependencies changed', () {
expect(useEffectWithDepsCallCount, 2, reason: 'count.value changed');
expect(useEffectCleanupWithDepsCallCount, 1, reason: 'cleanup called before re-render');
});

test('side effect (with dependency list) is not called again if none of its dependencies changed', () {
expect(useEffectWithDepsCallCount2, 1, reason: 'countDown.value did not change');
expect(useEffectCleanupWithDepsCallCount2, 0,
reason: 'cleanup not called because countDown.value did not change');
});

test('side effect (with empty dependency list) is not called again', () {
expect(useEffectWithEmptyDepsCallCount, 1,
reason: 'side effect is only called once for empty dependency list');
expect(useEffectCleanupWithEmptyDepsCallCount, 0, reason: 'component has not been unmounted');
});
});

group('after component is unmounted,', () {
setUpAll(() {
react_dom.unmountComponentAtNode(mountNode);
});

test('cleanup (no dependency list) is called', () {
expect(useEffectCallCount, 2, reason: 'side effect not called on unmount');
expect(useEffectCleanupCallCount, 2);
});

test('cleanup (with dependency list) is called', () {
expect(useEffectWithDepsCallCount, 2, reason: 'side effect not called on unmount');
expect(useEffectCleanupWithDepsCallCount, 2);

expect(useEffectWithDepsCallCount2, 1, reason: 'side effect not called on unmount');
expect(useEffectCleanupWithDepsCallCount2, 1);
});

test('cleanup (with empty dependency list) is called', () {
expect(useEffectWithEmptyDepsCallCount, 1, reason: 'side effect not called on unmount');
expect(useEffectCleanupWithEmptyDepsCallCount, 1);
});
});
});

group('useCallback -', () {
ReactDartFunctionComponentFactoryProxy UseCallbackTest;
DivElement deltaRef;
Expand Down Expand Up @@ -166,10 +318,6 @@ main() {
react_dom.render(UseCallbackTest({}), mountNode);
});

tearDownAll(() {
UseCallbackTest = null;
});

test('callback is called correctly', () {
expect(countRef.text, '0');
expect(deltaRef.text, '1');
Expand Down