UIP-2114: Update FluxUiComponent subscriptions when props are updated #77
Conversation
greglittlefield-wf
requested review from
maxwellpeterson-wf,
evanweible-wf and
trentgrover-wf
RavenNumber of Findings: 0 |
Codecov Report
@@ Coverage Diff @@
## master #77 +/- ##
==========================================
- Coverage 97.63% 97.58% -0.05%
==========================================
Files 28 28
Lines 1392 1402 +10
==========================================
+ Hits 1359 1368 +9
- Misses 33 34 +1 |
| /// respective handlers. | ||
| void _bindStoreHandlers() { | ||
| if (_areStoreHandlersBound) { | ||
| throw new StateError('Store handlers are already bound'); |
There was a problem hiding this comment.
@greglittlefield-wf does this need a unit test?
There was a problem hiding this comment.
The only case this would be hit is if someone doesn't add a super-call.
I wasn't sure if it was worth adding a test around, but I can add one if you'd like.
|
|
||
| /// Subscribe to all applicable stores. | ||
| /// | ||
| /// [Store]s returned by [redrawOn] will have their triggers mapped directly to this components |
| var stores = new TestStores(); | ||
| var newStores = new TestStores(); | ||
|
|
||
| var testJacket = mount<TestRedrawOnComponent>((TestRedrawOn()..store = stores)()); |
| @override | ||
| void componentWillReceiveProps(Map prevProps) { | ||
| // Unbind store handlers so they can be re-bound in componentDidUpdate | ||
| // once the new props are available, ensuring the values returned [redrawOn] |
UIP-2114
| // Unbind store handlers so they can be re-bound in componentDidUpdate | ||
| // once the new props are available, ensuring the values returned by [redrawOn] | ||
| // are not outdated. | ||
| _unbindStoreHandlers(); |
There was a problem hiding this comment.
Would it be worth only doing this if prevProps.store != props.store?
There was a problem hiding this comment.
We'd have to compare the equality of the return value of redrawOn and getStoreHandlers, and we don't have access to those updated values until componentDidUpdate (since those functions often rely on values within props). So, we'd have to store the old values in here and then check them and null them out in componentDidUpdate.
We can do that, but I figured that unbinding/rebinding was cheap enough that it was worth doing so to avoid that kind of comparison.
…ps_update/dev # Conflicts: # lib/src/component_declaration/flux_component.dart
|
QA +1
Merging. |
cherry-pick 009701b
Ultimate problem:
If a
FluxUiComponentis rerendered with differentprops.storeor different props that affect the return value ofredrawOn, it does not currently update its subscriptions to look at the latest store. This can happen indirectly as a result of rerendering larger views.How it was fixed:
mustCallSuperannotations to help consumers detect when they're interfering with existing mount/unmount hooks, as well as the new lifecycle hooks added to support this new behaviorcomponentWillReceiveProps/componentDidUpdate, then they just won't get this improved behavior. However, they may hit state issues if they call super on one and not the other.Testing suggestions:
Potential areas of regression:
FluxUiComponentrerendering