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

Stateful component mixin #38

Closed
Show file tree
Hide file tree
Changes from all 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
41 changes: 31 additions & 10 deletions lib/src/component_declaration/component_base.dart
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ typedef TProps BuilderOnlyUiFactory<TProps extends UiProps>();
///
/// Extends [react.Component].
///
/// Related: [UiStatefulComponent]
/// For strongly-typed state, mix in [UiStatefulMixin] or extend from [UiStatefulComponent].
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be fair to say that using UiStatefulMixin is considered best practice, instead of extending UiStatefulComponent? If so should we just deprecate UiStatefulComponent?

abstract class UiComponent<TProps extends UiProps> extends react.Component {
/// The props for the non-forwarding props defined in this component.
Iterable<ConsumedProps> get consumedProps => null;
Expand Down Expand Up @@ -193,16 +193,37 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component {
// END Typed props helpers
// ----------------------------------------------------------------------
// ----------------------------------------------------------------------


// ----------------------------------------------------------------------
// ----------------------------------------------------------------------
// BEGIN Typed state helpers
//

/// The state Map that will be used to create the typed [state] object.
///
/// Defined in [UiComponent] and not [UiStatefulMixin] to work around dart2js restriction
/// on super calls in mixins <https://github.com/dart-lang/sdk/issues/23773>.
Map get unwrappedState => super.state;
set unwrappedState(Map value) => super.state = value;

//
// END Typed state helpers
// ----------------------------------------------------------------------
// ----------------------------------------------------------------------
}

/// The basis for a stateful over_react component.
/// A base class for a stateful over_react component.
///
/// Includes support for strongly-typed props and state and utilities for prop and CSS classname forwarding.
///
/// Extends [react.Component].
///
/// Related: [UiComponent]
abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiState> extends UiComponent<TProps> {
abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiState>
extends UiComponent<TProps> with UiStatefulMixin<TState> {}

/// A mixin that adds support for strongly-typed state to a [UiComponent].
abstract class UiStatefulMixin<TState extends UiState>
// Implement react.Component instead of UiComponent so we don't run into https://github.com/dart-lang/sdk/issues/14729
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be cleaner to notate this in the doc comment instead?

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 don't think so, unless you think this information is beneficial to the audience of the doc comment.

// and have to pass through UiComponent's generic parameter.
implements react.Component {
// ----------------------------------------------------------------------
// ----------------------------------------------------------------------
// BEGIN Typed state helpers
Expand All @@ -225,11 +246,11 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
}
/// Equivalent to setting [unwrappedState], but needed by react-dart to effect props changes.
@override
set state(Map value) => super.state = value;
set state(Map value) => unwrappedState = value;

/// The state Map that will be used to create the typed [state] object.
Map get unwrappedState => super.state;
set unwrappedState(Map value) => super.state = value;
Map get unwrappedState;
set unwrappedState(Map value);

/// Returns a typed state object backed by the specified [stateMap].
///
Expand Down
36 changes: 15 additions & 21 deletions lib/src/component_declaration/flux_component.dart
Original file line number Diff line number Diff line change
Expand Up @@ -49,32 +49,13 @@ abstract class FluxUiProps<ActionsT, StoresT> extends UiProps {
/// the resulting component.
///
/// Use with the over_react transformer via the `@Component()` ([annotations.Component]) annotation.
abstract class FluxUiComponent<TProps extends FluxUiProps> extends UiComponent<TProps>
with _FluxComponentMixin<TProps>, BatchedRedraws {}

/// Builds on top of [UiStatefulComponent], adding `w_flux` integration, much like the [FluxComponent] in w_flux.
///
/// * Flux components are responsible for rendering application views and turning
/// user interactions and events into [Action]s.
/// * Flux components can use data from one or many [Store] instances to define
/// the resulting component.
///
/// Use with the over_react transformer via the `@Component()` ([annotations.Component]) annotation.
abstract class FluxUiStatefulComponent<TProps extends FluxUiProps, TState extends UiState>
extends UiStatefulComponent<TProps, TState>
with _FluxComponentMixin<TProps>, BatchedRedraws {}

/// Helper mixin to keep [FluxUiComponent] and [FluxUiStatefulComponent] clean/DRY.
///
/// 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 {
TProps get props;

abstract class FluxUiComponent<TProps extends FluxUiProps> extends UiComponent<TProps> with BatchedRedraws {
/// List of store subscriptions created when the component mounts.
///
/// These subscriptions are canceled when the component is unmounted.
List<StreamSubscription> _subscriptions = [];

@override
void componentWillMount() {
/// Subscribe to all applicable stores.
///
Expand All @@ -92,6 +73,7 @@ abstract class _FluxComponentMixin<TProps extends FluxUiProps> implements Batche
});
}

@override
void componentWillUnmount() {
// Ensure that unmounted components don't batch render
shouldBatchRedraw = false;
Expand Down Expand Up @@ -147,3 +129,15 @@ abstract class _FluxComponentMixin<TProps extends FluxUiProps> implements Batche
_subscriptions.add(subscription);
}
}

/// A [FluxUiComponent] subclass with typed state added via [UiStatefulMixin], for convenience.
///
/// * Flux components are responsible for rendering application views and turning
/// user interactions and events into [Action]s.
/// * Flux components can use data from one or many [Store] instances to define
/// the resulting component.
///
/// Use with the over_react transformer via the `@Component()` ([annotations.Component]) annotation.
abstract class FluxUiStatefulComponent<TProps extends FluxUiProps, TState extends UiState>
extends FluxUiComponent<TProps> with UiStatefulMixin<TState>
implements UiStatefulComponent<TProps, TState> {}
27 changes: 7 additions & 20 deletions lib/src/component_declaration/transformer_helpers.dart
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ class GeneratedClass {
/// See: [component_base.UiComponent]
///
/// Use with the over_react transformer via the `@Component()` ([annotations.Component]) annotation.
///
/// For strongly-typed state, mix in [component_base.UiStatefulMixin] or extend from [UiStatefulComponent].
abstract class UiComponent<TProps extends UiProps> extends component_base.UiComponent<TProps> with GeneratedClass {
/// This class should not be instantiated directly, and throws an error to indicate this.
UiComponent() {
Expand Down Expand Up @@ -118,34 +120,19 @@ abstract class UiComponent<TProps extends UiProps> extends component_base.UiComp
///
/// Use with the over_react transformer via the `@Component()` ([annotations.Component]) annotation.
abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiState>
extends component_base.UiStatefulComponent<TProps, TState> with GeneratedClass {
extends UiComponent<TProps> with component_base.UiStatefulMixin<TState>
implements component_base.UiStatefulComponent<TProps, TState> {
/// This class should not be instantiated directly, and throws an error to indicate this.
UiStatefulComponent() {
_throwIfNotGenerated();
}

/// The default consumed prop keys, taken from the keys generated in the associated @[annotations.Props] class.
@toBeGenerated
Iterable<component_base.ConsumedProps> get $defaultConsumedProps => throw new UngeneratedError(member: #$defaultConsumedProps);

/// The keys for the non-forwarding props defined in this component.
///
/// For generated components, this defaults to the keys generated in the associated @[annotations.Props] class
/// if this getter is not overridden.
@override
Iterable<component_base.ConsumedProps> get consumedProps => $defaultConsumedProps;

/// Returns a typed props object backed by the specified [propsMap].
///
/// Required to properly instantiate the generic [TProps] class.
@override
@toBeGenerated
TProps typedPropsFactory(Map propsMap) => throw new UngeneratedError(member: #typedPropsFactory);

/// Returns a typed state object backed by the specified [stateMap].
///
/// Required to properly instantiate the generic [TState] class.
@override @toBeGenerated TState typedStateFactory(Map stateMap) => throw new UngeneratedError(member: #typedStateFactory);
@override
@toBeGenerated
TState typedStateFactory(Map stateMap) => throw new UngeneratedError(member: #typedStateFactory);
}

/// A [dart.collection.MapView]-like class with strongly-typed getters/setters for React props that
Expand Down
1 change: 1 addition & 0 deletions smithy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ runner_image: drydock-prod.workiva.org/workiva/smithy-runner-dart:74173

script:
- pub get
- ./tool/smithy_dart2js_tests.sh

artifacts:
build:
Expand Down
2 changes: 1 addition & 1 deletion test/over_react/component/dom_components_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ library dom_components_test;
// Tell dart2js that this library only needs to reflect the specified types.
// This speeds up compilation and makes JS output much smaller.
@MirrorsUsed(targets: const [
'over_react.Dom'
'over_react.dom_components.Dom'
])
import 'dart:mirrors';

Expand Down
6 changes: 5 additions & 1 deletion tool/dev.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ main(List<String> args) async {
..pubServe = true
..platforms = [
'vm',
'content-shell'
'content-shell',
// Can't run tests in dart2js on Travis since the suite takes too long to load and times out.
// Run on Smithy instead.
// See https://github.com/Workiva/over_react/issues/36
// 'chrome',
]
// Prevent test load timeouts on Smithy.
..concurrency = 1
Expand Down
18 changes: 18 additions & 0 deletions tool/smithy_dart2js_tests.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#!/usr/bin/env bash

# Can't run tests in dart2js on Travis since the suite takes too long to load and times out.
# Run on Smithy instead.
# See https://github.com/Workiva/over_react/issues/36

set -e

# Trick the test package into using Chromium instead of Chrome
TMP_BIN=$(mktemp -d)
ln -s "$(which chromium-browser)" "$TMP_BIN/google-chrome"
export PATH="$PATH:$TMP_BIN"

# Run the tests
DART_FLAGS=--checked xvfb-run -s '-screen 0 1024x768x24' pub run dart_dev test -p chrome

# Be sneaky and clean up our tricks
rm -rf "$TMP_BIN"