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

Add an option to make BuiltReduxUiComponent "pure" #140

Merged
Show file tree
Hide file tree
Changes from 1 commit
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
35 changes: 31 additions & 4 deletions lib/src/experimental/built_redux_component.dart
Original file line number Diff line number Diff line change
Expand Up @@ -61,21 +61,28 @@ abstract class BuiltReduxUiComponent<
@override
void componentWillMount() {
super.componentWillMount();
_isDirty = false;
_setUpSub();
}

@mustCallSuper
@override
void componentWillReceiveProps(Map nextProps) {
super.componentWillReceiveProps(nextProps);
_tearDownSub();
var tNextProps = typedPropsFactory(nextProps);

if (tNextProps.store != props.store) {
_tearDownSub();
_setUpSub(nextProps);
}
}

@mustCallSuper
@override
void componentWillUpdate(Map nextProps, Map nextState) {
// _storeSub will only be null when props get updated, not on every re-render.
if (_storeSub == null) _setUpSub(nextProps);
bool shouldComponentUpdate(Map nextProps, Map nextState) {
if (isPure) return _isDirty || typedPropsFactory(nextProps).store != props.store;

return true;
}

@mustCallSuper
Expand All @@ -85,6 +92,19 @@ abstract class BuiltReduxUiComponent<
_tearDownSub();
}

@override
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have a mustCallSuper?

void redraw([callback()]) {
_isDirty = true;

super.redraw(() {
_isDirty = false;

if (callback != null) callback();
});
}

bool _isDirty;

Substate _connectedState;

/// The substate values of the redux store that this component subscribes to.
Expand Down Expand Up @@ -145,6 +165,13 @@ abstract class BuiltReduxUiComponent<
/// Related: [connectedState]
Substate connect(V state);

/// Whether the component should be a "pure" component.
///
/// A "pure" component will only re-render when [connectedState] is updated or [redraw] is called directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Or when the store changes, right?

Should there also be a note showing usage of this? For example:

/// To enable this functionality, override this getter in a subclass to return `true`.
///
///  When using this, do not override [redraw] or [shouldComponentUpdate].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

///
/// Related: [shouldComponentUpdate]
bool get isPure => false;

StreamSubscription _storeSub;

void _setUpSub([Map propsMap]) {
Expand Down
26 changes: 26 additions & 0 deletions test/over_react/experimental/redux_component_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import 'redux_component_test/test_reducer.dart';

part 'redux_component_test/default.dart';
part 'redux_component_test/connect.dart';
part 'redux_component_test/pure.dart';

void main() {
ReducerBuilder<BaseState, BaseStateBuilder> baseReducerBuilder;
Expand Down Expand Up @@ -95,6 +96,31 @@ void main() {
expect(component.numberOfRedraws, 1);
});

test('properly redraws when isPure is true', () async {
var store = new Store<BaseState, BaseStateBuilder, BaseActions>(
baseReducerBuilder.build(),
baseState,
baseActions,
);
var jacket = mount<TestDefaultComponent>((TestDefault()..store = store)());
TestDefaultComponent component = jacket.getDartInstance();

store.actions.trigger1();
await new Future.delayed(Duration.ZERO);
expect(component.numberOfRedraws, 1);

jacket.rerender((TestDefault()
..store = store
..id = 'new id'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the purpose of this ID to change some unrelated props when rerendering? If so, it might be nice to call that out explicitly, perhaps in the reason of the next expect.

expect(component.numberOfRedraws, 1, 
    reason: 'should not redraw when rerendered, even if other props change');

)());

expect(component.numberOfRedraws, 1);

component.redraw();

expect(component.numberOfRedraws, 2);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

There should also be a test that it updates as expected when props.store changes.


test('updates subscriptions when new props are passed', () async {
var store = new Store<BaseState, BaseStateBuilder, BaseActions>(
baseReducerBuilder.build(),
Expand Down
29 changes: 29 additions & 0 deletions test/over_react/experimental/redux_component_test/pure.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// ignore_for_file: deprecated_member_use

part of over_react.component_declaration.redux_component_test;

@Factory()
UiFactory<TestPureProps> TestPure;

@Props()
class TestPureProps extends BuiltReduxUiProps<BaseState, BaseStateBuilder, BaseActions> {}

@Component()
class TestPureComponent extends BuiltReduxUiComponent<BaseState, BaseStateBuilder, BaseActions, TestPureProps, BaseState> {
int numberOfRedraws = 0;

@override
bool get isPure => true;

@override
BaseState connect(BaseState state) => state;

@override
render() => Dom.div()();

@override
void setState(_, [callback()]) {
numberOfRedraws++;
if (callback != null) callback();
}
}