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

HY-4957 Alert devs about batchedRedraw race condition, and log if it happens in prod. ⚠️ #72

Merged
Show file tree
Hide file tree
Changes from 5 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
10 changes: 10 additions & 0 deletions lib/src/component_declaration/flux_component.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
library over_react.component_declaration.flux_component;

import 'dart:async';
import 'package:logging/logging.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add logging to pubspec.yaml?

import 'package:w_flux/w_flux.dart';
import './annotations.dart' as annotations;
import './transformer_helpers.dart';
Expand Down Expand Up @@ -82,6 +83,7 @@ abstract class FluxUiStatefulComponent<TProps extends FluxUiProps, TState extend
///
/// Private so it will only get used in this file, since having lifecycle methods in a mixin is risky.
abstract class _FluxComponentMixin<TProps extends FluxUiProps> implements BatchedRedraws {
final Logger _logger = new Logger('_FluxComponentMixin');

Choose a reason for hiding this comment

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

There's not clear guidance that I can find, but I think we should clearListeners on the Logger when things are disposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mm, good idea; we don't want this potentially leaking for every FluxComponent instance.

@tomconnell-wf What about making this instance static / top-level instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or cleaning it up in componentWillUnmount (shrug)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

TProps get props;

/// List of store subscriptions created when the component mounts.
Expand All @@ -101,6 +103,14 @@ abstract class _FluxComponentMixin<TProps extends FluxUiProps> implements Batche
value: (_) => (_) => redraw())..addAll(getStoreHandlers());

handlers.forEach((store, handler) {
String message = 'Cannot listen to a disposed/disposing Store.';
assert(!store.isDisposedOrDisposing, '$message This can be caused by BatchedRedraws '
'mounting the component asynchronously after the store has been disposed. If you are '
'in a test environment, try adding an `await window.animationFrame;` before disposing your '
'store.');

if (store.isDisposedOrDisposing) _logger.warning(message);

StreamSubscription subscription = store.listen(handler);
_subscriptions.add(subscription);
});
Expand Down
1 change: 1 addition & 0 deletions pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ dependencies:
analyzer: ">=0.30.0 <0.31.0"
barback: "^0.15.0"
js: "^0.6.0"
logging: ">=0.11.3+1 <1.0.0"
meta: "^1.0.4"
path: "^1.4.1"
react: "^3.1.0"
Expand Down