-
Notifications
You must be signed in to change notification settings - Fork 58
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -85,6 +92,20 @@ abstract class BuiltReduxUiComponent< | |
_tearDownSub(); | ||
} | ||
|
||
@mustCallSuper | ||
@override | ||
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. | ||
|
@@ -145,6 +166,15 @@ 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or when the 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]. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed |
||
/// To enable this functionality, override this getter in a subclass to return `true`. When set to true it | ||
/// is not recommended to override [redraw] or [shouldComponentUpdate]. | ||
/// | ||
/// Related: [shouldComponentUpdate], [redraw] | ||
bool get isPure => false; | ||
|
||
StreamSubscription _storeSub; | ||
|
||
void _setUpSub([Map propsMap]) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -95,6 +96,94 @@ void main() { | |
expect(component.numberOfRedraws, 1); | ||
}); | ||
|
||
group('properly redraws when isPure is true', () { | ||
test('when an action is triggered', () async { | ||
var store = new Store<BaseState, BaseStateBuilder, BaseActions>( | ||
baseReducerBuilder.build(), | ||
baseState, | ||
baseActions, | ||
); | ||
var jacket = mount<TestPureComponent>((TestPure()..store = store)()); | ||
TestPureComponent component = jacket.getDartInstance(); | ||
|
||
store.actions.trigger1(); | ||
await new Future.delayed(Duration.ZERO); | ||
expect(component.numberOfRedraws, 1); | ||
}); | ||
|
||
test('by not redrawing when other props change', () async { | ||
var store = new Store<BaseState, BaseStateBuilder, BaseActions>( | ||
baseReducerBuilder.build(), | ||
baseState, | ||
baseActions, | ||
); | ||
var jacket = mount<TestPureComponent>((TestPure()..store = store)()); | ||
TestPureComponent component = jacket.getDartInstance(); | ||
|
||
expect(component.numberOfRedraws, 0); | ||
|
||
jacket.rerender((TestPure() | ||
..store = store | ||
..id = 'new id' | ||
)()); | ||
|
||
expect(component.numberOfRedraws, 0); | ||
}); | ||
|
||
test('by redrawing when store changes', () async { | ||
var store = new Store<BaseState, BaseStateBuilder, BaseActions>( | ||
baseReducerBuilder.build(), | ||
baseState, | ||
baseActions, | ||
); | ||
var updatedStore = new Store<BaseState, BaseStateBuilder, BaseActions>( | ||
baseReducerBuilder.build(), | ||
new BaseState(), | ||
new BaseActions(), | ||
); | ||
var jacket = mount<TestPureComponent>((TestPure()..store = store)()); | ||
TestPureComponent component = jacket.getDartInstance(); | ||
|
||
expect(component.numberOfRedraws, 0); | ||
|
||
jacket.rerender((TestPure()..store = updatedStore)()); | ||
|
||
expect(component.numberOfRedraws, 1); | ||
}); | ||
|
||
test('when calling redraw', () { | ||
var store = new Store<BaseState, BaseStateBuilder, BaseActions>( | ||
baseReducerBuilder.build(), | ||
baseState, | ||
baseActions, | ||
); | ||
var jacket = mount<TestPureComponent>((TestPure()..store = store)()); | ||
TestPureComponent component = jacket.getDartInstance(); | ||
|
||
expect(component.numberOfRedraws, 0); | ||
|
||
component.redraw(); | ||
|
||
expect(component.numberOfRedraws, 1); | ||
}); | ||
|
||
test('when calling redraw', () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #nit this looks like a duplicate test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed |
||
var store = new Store<BaseState, BaseStateBuilder, BaseActions>( | ||
baseReducerBuilder.build(), | ||
baseState, | ||
baseActions, | ||
); | ||
var jacket = mount<TestPureComponent>((TestPure()..store = store)()); | ||
TestPureComponent component = jacket.getDartInstance(); | ||
|
||
expect(component.numberOfRedraws, 0); | ||
|
||
component.redraw(); | ||
|
||
expect(component.numberOfRedraws, 1); | ||
}); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should also be a test that it updates as expected when |
||
|
||
test('updates subscriptions when new props are passed', () async { | ||
var store = new Store<BaseState, BaseStateBuilder, BaseActions>( | ||
baseReducerBuilder.build(), | ||
|
@@ -115,9 +204,11 @@ void main() { | |
|
||
jacket.rerender((TestDefault()..store = updatedStore)()); | ||
|
||
expect(component.numberOfRedraws, 2); | ||
|
||
updatedStore.actions.trigger1(); | ||
await new Future.delayed(Duration.ZERO); | ||
expect(component.numberOfRedraws, 2); | ||
expect(component.numberOfRedraws, 3); | ||
}); | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
// 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 | ||
void componentDidUpdate(Map prevProps, Map prevState) { | ||
numberOfRedraws++; | ||
} | ||
|
||
@override | ||
bool get isPure => true; | ||
|
||
@override | ||
BaseState connect(BaseState state) => state; | ||
|
||
@override | ||
render() => Dom.div()(); | ||
} |
There was a problem hiding this comment.
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
?