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

UIP-2161 Implement DisposableManagerV3 for UiComponent #91

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
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
1 change: 1 addition & 0 deletions lib/src/component/abstract_transition.dart
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ abstract class AbstractTransitionComponent<T extends AbstractTransitionProps,
@mustCallSuper
@override
void componentWillUnmount() {
super.componentWillUnmount();
_isUnmounted = true;
_cancelTransitionEventListener();
}
Expand Down
72 changes: 70 additions & 2 deletions lib/src/component_declaration/component_base.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

library over_react.component_declaration.component_base;

import 'dart:async';

import 'package:meta/meta.dart';
import 'package:over_react/over_react.dart' show
ClassNameBuilder,
Expand All @@ -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';

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created RAP-2130 to address this.


export 'package:over_react/src/component_declaration/component_type_checking.dart' show isComponentOfType, isValidElementOfType;

Expand Down Expand Up @@ -94,8 +97,12 @@ typedef TProps BuilderOnlyUiFactory<TProps extends UiProps>();
///
/// Extends [react.Component].
///
/// Implements [DisposableManagerV3]
///
/// Related: [UiStatefulComponent]
abstract class UiComponent<TProps extends UiProps> extends react.Component {
abstract class UiComponent<TProps extends UiProps> extends react.Component implements DisposableManagerV3 {
Disposable _disposableProxy;

/// The props for the non-forwarding props defined in this component.
Iterable<ConsumedProps> get consumedProps => null;

Expand Down Expand Up @@ -156,6 +163,12 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component {
validateRequiredProps(props);
}

@override
@mustCallSuper
void componentWillUnmount() {
_disposableProxy?.dispose();
}


// ----------------------------------------------------------------------
// ----------------------------------------------------------------------
Expand Down Expand Up @@ -199,6 +212,61 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component {
// END Typed props helpers
// ----------------------------------------------------------------------
// ----------------------------------------------------------------------

// ----------------------------------------------------------------------
// ----------------------------------------------------------------------
// BEGIN DisposableManagerV3 interface implementation
//

@override
Future<T> awaitBeforeDispose<T>(Future<T> future) =>
_getDisposableProxy().awaitBeforeDispose<T>(future);

@override
Future<T> getManagedDelayedFuture<T>(Duration duration, T callback()) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make the callback param covariant?

_getDisposableProxy().getManagedDelayedFuture<T>(duration, callback);

@override
Timer getManagedPeriodicTimer(Duration duration, void callback(Timer timer)) =>
_getDisposableProxy().getManagedPeriodicTimer(duration, callback);

@override
Timer getManagedTimer(Duration duration, void callback()) =>
_getDisposableProxy().getManagedTimer(duration, callback);

@override
Completer<T> manageCompleter<T>(Completer<T> completer) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make the parameter covariant?

_getDisposableProxy().manageCompleter<T>(completer);

@override
void manageDisposable(Disposable disposable) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make the parameter covariant?

_getDisposableProxy().manageDisposable(disposable);

@override
void manageDisposer(Disposer disposer) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make the parameter covariant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

_getDisposableProxy().manageDisposer(disposer);

@override
void manageStreamController(StreamController controller) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make the parameter covariant?

_getDisposableProxy().manageStreamController(controller);

@override
void manageStreamSubscription(StreamSubscription subscription) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make the parameter covariant?

_getDisposableProxy().manageStreamSubscription(subscription);

/// Instantiates a new [Disposable] instance on the first call to the
/// [DisposableManagerV3] method.
Disposable _getDisposableProxy() {
if (_disposableProxy == null) {
_disposableProxy = new Disposable();
}
return _disposableProxy;
}

//
// END DisposableManagerV3 interface implementation
// ----------------------------------------------------------------------
// ----------------------------------------------------------------------
}

/// The basis for a stateful over_react component.
Expand Down Expand Up @@ -248,7 +316,7 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
TState newState() => typedStateFactory({});

//
// END Typed state helpers
// END Typed props helpers
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this got changed accidentally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😰

// ----------------------------------------------------------------------
// ----------------------------------------------------------------------
}
Expand Down
4 changes: 4 additions & 0 deletions lib/src/component_declaration/flux_component.dart
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ abstract class FluxUiComponent<TProps extends FluxUiProps> extends UiComponent<T

@mustCallSuper
@override
// Ignore this warning to work around https://github.com/dart-lang/sdk/issues/29860
// ignore: must_call_super
void componentWillUnmount();
}

Expand Down Expand Up @@ -124,6 +126,8 @@ abstract class FluxUiStatefulComponent<TProps extends FluxUiProps, TState extend

@mustCallSuper
@override
// Ignore this warning to work around https://github.com/dart-lang/sdk/issues/29860
// ignore: must_call_super
void componentWillUnmount();
}

Expand Down
1 change: 1 addition & 0 deletions pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ dependencies:
react: "^3.4.1"
source_span: "^1.4.0"
transformer_utils: "^0.1.1"
w_common: "^1.6.0"
w_flux: "^2.7.1"
platform_detect: "^1.3.2"
quiver: ">=0.21.4 <0.26.0"
Expand Down
121 changes: 121 additions & 0 deletions test/over_react/component_declaration/component_base_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@

library over_react.component_declaration.component_base_test;

import 'dart:async';
import 'dart:html';

import 'package:over_react/over_react.dart' show Dom, DummyComponent, ValidationUtil;
import 'package:over_react/src/component_declaration/component_base.dart';
import 'package:over_react/src/component_declaration/component_type_checking.dart';
import 'package:react/react_client.dart';
import 'package:test/test.dart';
import 'package:w_common/disposable.dart';

import '../../test_util/test_util.dart';
import '../../wsd_test_util/validation_util_helpers.dart';
Expand Down Expand Up @@ -623,6 +625,125 @@ main() {
expect(classes.toClassName(), equals('class-1'));
expect(classes.toClassNameBlacklist(), equals('blacklist-1'));
});

group('on unmount', () {
TestComponentComponent component;
ReactElement instance;
Duration longDuration = const Duration(milliseconds: 200);
Duration shortDuration = const Duration(milliseconds: 100);

setUp(() {
instance = render(TestComponent()());
component = getDartComponent(instance);
});

Future<Null> unmountAndDisposal() async {
unmount(instance);
// Provide timers a window to fire
await new Future.delayed(longDuration);
}

test('should await future before disposing', () async {
// ignore: close_sinks
var streamController = new StreamController<String>.broadcast();
var completer = new Completer<String>();

// Manage pending future
component.awaitBeforeDispose(completer.future);

// Add events to stream
component.manageDisposer(() async => streamController.add('disposalFuture'));
completer.future.then(streamController.add);

// Perform events out of order
await unmountAndDisposal();
completer.complete('awaitedFuture');

// Ensure events resolve in the correct order
expect(streamController.stream, emitsInOrder([
'awaitedFuture',
'disposalFuture',
]));
});

test('should complete delayed Future with ObjectDisposedException', () async {
expect(component.getManagedDelayedFuture(shortDuration,
expectAsync0(() {}, count: 0, reason: 'Did not expect callback to be invoked.')),
throwsA(new isInstanceOf<ObjectDisposedException>()));

await unmountAndDisposal();
});

test('should cancel periodic timer', () async {
var timer = component.getManagedPeriodicTimer(shortDuration,
expectAsync1((_) {}, count: 0, reason: 'Did not expect callback to be invoked.'));

expect(timer.isActive, isTrue);
await unmountAndDisposal();
expect(timer.isActive, isFalse);
});

test('should cancel timer', () async {
var timer = component.getManagedTimer(shortDuration,
expectAsync1((_){}, count: 0, reason: 'Did not expect callback to be invoked.'));

expect(timer.isActive, isTrue);
await unmountAndDisposal();
expect(timer.isActive, isFalse);
});

test('should complete uncompleted managed Completer with ObjectDisposedException', () async {
var completer = new Completer<Null>();
component.manageCompleter(completer);
completer.future.catchError(expectAsync1((Object err) =>
expect(err, new isInstanceOf<ObjectDisposedException>())));

expect(completer.isCompleted, isFalse);
await unmountAndDisposal();
expect(completer.isCompleted, isTrue);
});

test('should dispose managed Disposable', () async {
var disposable = new Disposable();
component.manageDisposable(disposable);
expect(disposable.isDisposed, isFalse);
await unmountAndDisposal();
expect(disposable.isDisposed, isTrue);
});

test('should call managed disposers', () async {
var disposerCalled = false;
component.manageDisposer(() async => disposerCalled = true);
expect(disposerCalled, isFalse);
await unmountAndDisposal();
expect(disposerCalled, isTrue);
});

test('should close managed StreamController', () async {
//ignore: close_sinks
var streamController = new StreamController<Null>.broadcast();
component.manageStreamController(streamController);
expect(streamController.isClosed, isFalse);
await unmountAndDisposal();
expect(streamController.isClosed, isTrue);
});

test('should cancel managed StreamSubscription', () async{
var streamController = new StreamController<Null>.broadcast();
// ignore: cancel_subscriptions
var streamSubscription = streamController.stream
.listen(expectAsync1((_) {},
count: 0,
reason: 'Did not expect event after cancelling subscription'));

component.manageStreamSubscription(streamSubscription);
await unmountAndDisposal();

streamController
..add(null)
..close();
});
}, timeout: new Timeout(const Duration(milliseconds: 250)));
});

group('UiStatefulComponent', () {
Expand Down