UIP-2161 Implement DisposableManagerV3 for UiComponent #91

bencampbell-wf · 2017-06-29T14:40:26Z

Ultimate problem:

Consumers are required to manually deal with lifecycle management within the component lifecycle. This either requires mixing lifecycle management functionality into all of their components or independently managing the lifetime of StreamControllers, `StreamSubscriptions, etc in each component.

How it was fixed:

The DisposableManagerV3 interface has been mixed into the base implementation of UiComponent. This interface proxies all calls to an internal instance of w_common's Disposable. The call to dispose is tied to the componentWillUnmount call ensuring that managed objects are correctly cancelled or concluded when the component is removed from the DOM.

Testing suggestions:

The disposable functionality has been exercised in the unit tests. The CI run should pass to confirm this functionality is correct.

Potential areas of regression:

An override of react.Component.componentWillUnmount was added to the UiComponent implementation with an annotation that super must be called. There is concern how this may affect current consumers extending UiComponent and are already overriding componentWillUnmount.

FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

All components now implement the DisposableManagerV3 interface proxying management calls to an internal instance of Disposable. This provides consumers with the set of w_common Disposable methods to ease the management of various object types. The disposal of these managed objects is triggered by the unmounting of the compoment.

aviary-wf · 2017-06-29T14:40:33Z

Raven

Number of Findings: 0

codecov-io · 2017-06-29T14:50:32Z

Codecov Report

Merging #91 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   94.81%   94.86%   +0.05%     
==========================================
  Files          31       31              
  Lines        1522     1536      +14     
==========================================
+ Hits         1443     1457      +14     
  Misses         79       79

aaronlademann-wf

@bencampbell-wf Looks really good!

Mainly just wondering if we should allow consumers to subclasses for some of the typed parameters without having to utilize the covariant keyword in their implementations.

I placed a comment on each so we can have a discussion around the merits of covariant for each one (since we may not want to do it for all of them).

aaronlademann-wf · 2017-06-29T15:17:25Z

lib/src/component_declaration/component_base.dart

+    _getDisposableProxy().awaitBeforeDispose<T>(future);
+
+  @override
+  Future<T> getManagedDelayedFuture<T>(Duration duration, T callback()) =>


Should we make the callback param covariant?

aaronlademann-wf · 2017-06-29T15:17:57Z

lib/src/component_declaration/component_base.dart

+    _getDisposableProxy().getManagedTimer(duration, callback);
+
+  @override
+  Completer<T> manageCompleter<T>(Completer<T> completer) =>


Should we make the parameter covariant?

aaronlademann-wf · 2017-06-29T15:18:37Z

lib/src/component_declaration/component_base.dart

+    _getDisposableProxy().manageCompleter<T>(completer);
+
+  @override
+  void manageDisposable(Disposable disposable) =>


Should we make the parameter covariant?

aaronlademann-wf · 2017-06-29T15:18:44Z

lib/src/component_declaration/component_base.dart

+    _getDisposableProxy().manageDisposable(disposable);
+
+  @override
+  void manageDisposer(Disposer disposer) =>


Should we make the parameter covariant?

I am unsure if/how inheritance would work in the case of typedefs. Does covariant make sense in that case?

oh... didn't realize Disposer was a typedef - not a class.

aaronlademann-wf · 2017-06-29T15:18:57Z

lib/src/component_declaration/component_base.dart

+    _getDisposableProxy().manageDisposer(disposer);
+
+  @override
+  void manageStreamController(StreamController controller) =>


Should we make the parameter covariant?

aaronlademann-wf · 2017-06-29T15:19:03Z

lib/src/component_declaration/component_base.dart

+    _getDisposableProxy().manageStreamController(controller);
+
+  @override
+  void manageStreamSubscription(StreamSubscription subscription) =>


Should we make the parameter covariant?

aaronlademann-wf · 2017-06-29T15:20:04Z

lib/src/component_declaration/component_base.dart

+  }
+
+  //
+  //   END Typed props helpers


#nit

// END DisposableManagerV3 interface implementation

aaronlademann-wf · 2017-06-29T15:20:26Z

lib/src/component_declaration/component_base.dart

@@ -248,7 +316,7 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
  TState newState() => typedStateFactory({});

  //
-  //   END Typed state helpers
+  //   END DisposableManagerV3 interface implementation


I believe this is accidental - and should be on line 267 instead?

bencampbell-wf · 2017-06-29T15:55:25Z

@aaronlademann-wf I don't see how covariant would hurt anything given the consumption of the DisposableManager interface is internal. At the same time I unable to see a use case where a consumer would need covariant. I'd be glad to make all the parameters covariant if that is a common pattern through the library.

greglittlefield-wf · 2017-06-29T18:22:38Z

@aaronlademann-wf I don't think there's a need to use use covariant, and adding it makes it easier for consumers to accidentally/unintentionally violate the liskov substitution principle.

If consumers need covariant, they can add it on their overrides.

greglittlefield-wf

+1 other than that one comment change. Looks great!! Thanks for implementing this, @bencampbell-wf !!

greglittlefield-wf · 2017-06-29T18:24:12Z

lib/src/component_declaration/component_base.dart

@@ -248,7 +316,7 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
  TState newState() => typedStateFactory({});

  //
-  //   END Typed state helpers
+  //   END Typed props helpers


Looks like this got changed accidentally

greglittlefield-wf · 2017-06-29T20:50:51Z

+1

robbielamb-wf · 2017-06-29T20:56:28Z

lib/src/component_declaration/component_base.dart

@@ -31,6 +33,7 @@ import 'package:over_react/src/component_declaration/component_type_checking.dar
 import 'package:over_react/src/util/ddc_emulated_function_name_bug.dart' as ddc_emulated_function_name_bug;
 import 'package:react/react.dart' as react;
 import 'package:react/react_client.dart';
+import 'package:w_common/disposable.dart';


should this pull in w_common/disposable_browser.dart instead of just disposable.dart?

It is dependent on updating the the DisposableManager interface to include the new browser specific utility functions.

I have created RAP-2130 to address this.

jacehensley-wf · 2017-07-05T17:04:38Z

+1

aaronlademann-wf · 2017-07-05T17:07:22Z

QA +10

Merging.

UIP-2161 Use correct identifier in section comment

6374a60

aaronlademann-wf added area-component_base State: needs review Type: new feature labels Jun 29, 2017

aaronlademann-wf reviewed Jun 29, 2017

View reviewed changes

bencampbell-wf added 2 commits June 29, 2017 08:38

UIP-2161 Add test for awaited futures

d62e5e3

UIP-2161 Revert section comment changes

542bd57

Workiva deleted a comment from jacehensley-wf Jun 29, 2017

greglittlefield-wf reviewed Jun 29, 2017

View reviewed changes

UIP-2161 Address copypasta mistake

c8a2e06

rmconsole-wf added the Merge Requirements Met label Jun 29, 2017

robbielamb-wf reviewed Jun 29, 2017

View reviewed changes

aaronlademann-wf added State: ready for QA and removed State: needs review labels Jul 5, 2017

aaronlademann-wf merged commit da284fd into Workiva:master Jul 5, 2017

bencampbell-wf deleted the uip-2161-disposable-manager branch July 5, 2017 17:13

aaronlademann-wf mentioned this pull request Jul 5, 2017

UIP-2458 Release over_react 1.14.0 #93

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIP-2161 Implement DisposableManagerV3 for UiComponent #91

UIP-2161 Implement DisposableManagerV3 for UiComponent #91

bencampbell-wf commented Jun 29, 2017

aviary-wf commented Jun 29, 2017

codecov-io commented Jun 29, 2017 •

edited

Loading

aaronlademann-wf left a comment

aaronlademann-wf Jun 29, 2017

aaronlademann-wf Jun 29, 2017

aaronlademann-wf Jun 29, 2017

aaronlademann-wf Jun 29, 2017

bencampbell-wf Jun 29, 2017

aaronlademann-wf Jun 29, 2017

aaronlademann-wf Jun 29, 2017

aaronlademann-wf Jun 29, 2017

aaronlademann-wf Jun 29, 2017

aaronlademann-wf Jun 29, 2017

bencampbell-wf commented Jun 29, 2017

greglittlefield-wf commented Jun 29, 2017

greglittlefield-wf left a comment

greglittlefield-wf Jun 29, 2017

bencampbell-wf Jun 29, 2017

greglittlefield-wf commented Jun 29, 2017

robbielamb-wf Jun 29, 2017

johnbland-wf Jun 29, 2017

bencampbell-wf Jun 30, 2017

bencampbell-wf Jun 30, 2017

jacehensley-wf commented Jul 5, 2017

aaronlademann-wf commented Jul 5, 2017

UIP-2161 Implement DisposableManagerV3 for UiComponent #91

UIP-2161 Implement DisposableManagerV3 for UiComponent #91

Conversation

bencampbell-wf commented Jun 29, 2017

Ultimate problem:

How it was fixed:

Testing suggestions:

Potential areas of regression:

aviary-wf commented Jun 29, 2017

Raven

codecov-io commented Jun 29, 2017 • edited Loading

Codecov Report

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bencampbell-wf commented Jun 29, 2017

greglittlefield-wf commented Jun 29, 2017

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacehensley-wf commented Jul 5, 2017

aaronlademann-wf commented Jul 5, 2017

codecov-io commented Jun 29, 2017 •

edited

Loading