Add BuiltReduxUiComponent #118

jacehensley-wf · 2017-10-03T23:15:02Z

Ultimate problem:

We should have support for the built_redux library.

How it was fixed:

Add BuiltReduxUiProps and BuiltReduxUiComponent that wire up listeners to the store's stream.

Testing suggestions:

Verify tests pass

Potential areas of regression:

N/A All new stuff

FYA: @greglittlefield-wf @aaronlademann-wf @clairesarsam-wf @joelleibow-wf
FYI: @davidmarne-wf

aviary2-wf · 2017-10-03T23:15:47Z

Raven

Number of Findings: 0

greglittlefield-wf · 2017-10-03T23:23:03Z

pubspec.yaml

@@ -9,6 +9,8 @@ environment:
 dependencies:
  analyzer: ">=0.30.0 <0.31.0"
  barback: "^0.15.0"
+  built_redux: ^6.1.0
+  built_value: ^4.2.0


Is this dependency needed?

Hmm I guess it's only needed for tests, will move

greglittlefield-wf · 2017-10-03T23:23:43Z

pubspec.yaml

@@ -9,6 +9,8 @@ environment:
 dependencies:
  analyzer: ">=0.30.0 <0.31.0"
  barback: "^0.15.0"
+  built_redux: ^6.1.0


This dependency worries me since it's undergoing major version bumps quite frequently. If we include this, that means we have to stay on top of bumping the upper bound to avoid being a source of version lock.

Yeah, that's a good point

It only looks that way because, per @davidmarne-wf, because he went 1.0 too early and broke some things so he had to rev for semver purposes.

Yeah, I understand your hesitation for sure, but I really do believe it finally settled down. I'm very happy with where it stands right now.

Plus at this point it is a dependency for CDS, w_comments, review_bar, and soon to be graph_app (and many other repos via transitivity from w_comments) so any major bumps are already going to have to be dealt with anyways.

However, if we want I could easy make another repo that just has this component.

codecov-io · 2017-10-03T23:24:11Z

Codecov Report

Merging #118 into master will increase coverage by 0.02%.
The diff coverage is 95.46%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   94.42%   94.44%   +0.02%     
==========================================
  Files          31       32       +1     
  Lines        1559     1581      +22     
==========================================
+ Hits         1472     1493      +21     
- Misses         87       88       +1

greglittlefield-wf · 2017-10-03T23:25:26Z

Is this set of base classes a pattern that's also being used in comments? Trying to get a feel for how mature this approach is, and whether more exploration is needed before committing to this API.

jacehensley-wf · 2017-10-03T23:26:10Z

I believe w_comments it just listening to the streams on mount. Let me double check

greglittlefield-wf · 2017-10-03T23:27:34Z

And to clarify, I'm not against this approach, I just want to make sure it's tried and true before adding it to this repo.

jacehensley-wf · 2017-10-03T23:28:09Z

@greglittlefield-wf You can see their usage here

aaronlademann-wf · 2017-10-04T13:58:46Z

lib/src/component_declaration/redux_component.dart

@@ -0,0 +1,163 @@
+// Copyright 2016 Workiva Inc.


s/2016/2017

Same goes for all files added in this PR

aaronlademann-wf · 2017-10-04T14:03:49Z

lib/src/component_declaration/redux_component.dart

+abstract class ReduxUiProps<StoreT> extends UiProps {
+  String get _storePropKey => '${propKeyNamespace}store';
+
+  /// The prop defined by [StoresT].


s/StoresT/StoreT

aaronlademann-wf · 2017-10-04T14:04:18Z

lib/src/component_declaration/redux_component.dart

+
+  /// The prop defined by [StoresT].
+  ///
+  /// This object should either be an instance of [Store].


either an instance of [Store] ... or ... what else? ;)

aaronlademann-wf · 2017-10-04T14:07:46Z

test/over_react/component_declaration/redux_component_test/test_reducer.g.dart

@@ -0,0 +1,222 @@
+// GENERATED CODE - DO NOT MODIFY BY HAND


@jacehensley-wf is it necessary to commit generated files?

If you don't commit, build it into your build process to build before tests run.

Yeah, I can just build it during CI, but while deving you'll have analysis warnings till you generate

aaronlademann-wf · 2017-10-04T14:08:09Z

tool/build.dart

+import 'package:source_gen/source_gen.dart';
+import 'package:built_redux/generator.dart';
+
+/// Build the generated files in the built_value chat example.


What chat example?

johnbland-wf · 2017-10-04T14:57:23Z

test/over_react/component_declaration/redux_component_test/default.dart

+
+  @override
+  void setState(_, [callback()]) {
+    print('here');


johnbland-wf · 2017-10-04T14:58:33Z

lib/src/component_declaration/redux_component.dart

+    // Unbind store subs so they can be re-bound in componentDidUpdate
+    // once the new props are available, ensuring the values returned by [redrawOn]
+    // are not outdated.
+    _tearDownSubs();


If the store prop is the same, we don't need new subs, right?

It's not worth checking that, it doesn't take much to teardown and re-bind the subs

I'm curious why it isn't worth it. It seems like you'd want to not force something that is not necessary.

Tearing down and rebinding subscriptions is cheap, I don't think it's necessary to expend effort to avoid it.

Also, we we'd have to compare the return value of redrawOn and not just props.store, and we don't have access to that updated return value until componentDidUpdate (since that function can 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.

In this case, unbinding/rebinding is more straightforward, and again, should cheap enough to be worth it.

I'm confused about this, tracking _areStoreSubsBound and having all these teardown and setups it seems overly complicated to me.
The reference to your store should never change, it seems to me that you would only setup subs on componentWillMount and remove them on componentWillUnmount.

johnbland-wf · 2017-10-04T15:03:03Z

@greglittlefield-wf, the preferred use in Redux.js is to use a HOC called a provider and connect the data primitive components. In Dart, we don't have that until react-dart supports context.

We're starting to use this in grc and we're looking at a wrapper component vs a base component. The typical flow, per David, is to have a wrapper component handle the subscription and wire up the child components from there. This PR would be used as base class(es) we extend for our wrapper component.

johnbland-wf · 2017-10-04T15:05:22Z

Oh and you really should bring over the pure component in Comments. If we're using built-redux, a PureReduxComponent should exist for everyone.

jacehensley-wf · 2017-10-04T16:26:52Z

@johnbland-wf We can't do the HOC until we add support for React's context to react-dart.

For the PureReduxComponent I could see that being useful but I'd rather just get this in for now, and adding something like that later.

davidmarne-wf · 2017-10-04T16:56:12Z

lib/src/component_declaration/redux_component.dart

+  }
+}
+
+abstract class _ReduxComponentMixin<T extends ReduxUiProps> implements UiComponent<T> {


I think it would make a lot of sense to have a connect function here. The default implementation could be something like

Substate connect(State state) => state;

But then that could be overridden to only redraw when specific pieces of your state change

@override Substate connect(State state) => state.myComponentWantsToRedrawOn;

then redrawOn would be something like

List<Stream> redrawOn() { if (props.store is Store) { return <Stream>[props.store.nextSubstate(connect)]; } else { return <Stream>[]; } }

With that how would someone listen to multiple stores?

There is no concept of multiple stores. It is one store in redux.

True, what about multiple values on the single store?

Agreed. We'll want a substate on multiple values.

So one thing you can do is map all the values into a new built_value. For example say by store's state object is:

abstract class AppState implements Built<AppState, AppStateBuilder> { int get foo; int get bar; int get baz; }

But my component only wants foo and bar. I can still only using a single nextSubstate by defining a new model that contains the subset of state my component cares about and having my connect function return that. So i would define

abstract class DataComponentWants implements Built<DataComponentWants, DataComponentWantsBuilder> { int get foo; int get bar; }

and have connect do:

DataComponentWants connect(AppState state) => new DataComponentWants( foo: state.foo, bar: state.bar, );

Since DataComponentWants is also a built, it should be comparable with the last result of connect, so nextSubstate will only trigger if the resulting value of DataComponentWants changes after the AppState change.

johnbland-wf · 2017-10-04T19:57:14Z

@jacehensley-wf yes (re: react context). I mentioned that in hopes @greglittlefield-wf would find it in his heart to push that forward. ;)

Agreed on PureReduxComponent being separate. Just getting the list out there. ;)

Note: It really is a PureImmutableComponent since it would work fine without redux so long as the props respond properly to == (ie - immutable or primitives).

greglittlefield-wf · 2017-10-04T22:24:42Z

My main concern was marching forward with this without thoroughly planning this feature, but it seems like there's a consensus that this is what we want 👍.

evanweible-wf · 2017-10-05T15:35:19Z

lib/src/component_declaration/redux_component.dart

+/// * Redux components are responsible for rendering application views and turning
+///   user interactions and events into [Action]s.
+/// * Redux components can use data from one or many [Store] instances to define
+///   the resulting component.


Just one store, right?

davidmarne-wf · 2017-10-09T18:07:21Z

I threw up a proposal on how I could see this working here:
https://github.com/davidmarne-wf/over_react/ pull number 1

The generics get a bit verbose, but it is necessary to get legit type safety.

jacehensley-wf · 2017-10-09T22:29:11Z

@davidmarne-wf How does this look?

johnbland-wf · 2017-10-12T15:31:33Z

lib/src/component_declaration/redux_component.dart

+    _storeSub = props.store.nextSubstate(connect).listen((Substate s) {
+      _connectedProps = s;
+      redraw();
+    });


I think the only problem with this is we can only listen to one substate so we'd be stuck if we wanted state.count and state.todos but not state.options.

Thoughts, @davidmarne-wf?

No, your substate would be a built_value object like shown in the doc comment https://github.com/Workiva/over_react/pull/118/files/20dc99def5af7766e5045dbcc909ad3bb39db8f1#diff-cd6ab537d255abbf1ec247df0369120eR119

i am of the opinion you define a new built_value and map state to that. so say your state is

abstract class State extends Built<State, StateBuilder> { int get count; BuiltList<Todo> todos; Map<String, String> options; ... }

you define a model

abstract class SubstateMyComponentWants extends Built<SubstateMyComponentWants, SubstateMyComponentWantsBuilder> { int get count; BuiltList<Todo> todos; ... }

then your connect is

@override SubstateMyComponentWants connect(State s) => new SubstateMyComponentWants((builder) => builder ..count = state.count ..todos = state.todos);

I'ld be open to suggestions for multiple connectors though. I imagine it would look something like:

typedef Substate StateMapper<State, Substate>(State s); Iterable<StateMapper<State, dynamic>> get connectors; ... void _setUpSub() { _storeSubs.addAll(connectors.map((connect) => props.store.nextSubstate(connect).listen((_) { redraw(); })); }

then you wouldn't have connectedState, you would prob just expose a store.state getter and let the renderers pull data from that.

or you could just have

bool shouldUpdateOnReduxStateChange(State prev, State next));

that the consumer implements.

then if i wanna update on todos or count changes i do:

@override bool shouldUpdateOnReduxStateChange(State prev, State next) => prev.todos != next.todos || prev.count != next.count

The upside to keeping how it is now, where you load everything into a new built_value, is this comparison is generated for the model.

The shouldUpdateOnReduxStateChange pattern would be less code when you are only pulling a couple things from the store, but the connect pattern would be less if you are mapping a lot of data from your store. Plus you are more likely to goof up shouldUpdateOnReduxStateChange and forget to check a param you want to update on

I like the first option, with using built_value objects to contain multiple state values

That's fine.

johnbland-wf · 2017-10-12T15:42:52Z

lib/src/component_declaration/redux_component.dart

+  ///       int get baz;
+  ///     }
+  ///
+  ///     abstract class DataCoponentCaresAbout implements Built<DataCoponentCaresAbout, DataCoponentCaresAboutBuilder> {


DataCoponent -> DataComponent

greglittlefield-wf

Some comments on test coverage and some other small things, otherwise looks really close!

Also, PR description needs updating (e.g., ReduxUiStatefulComponent is no longer included).

greglittlefield-wf · 2017-11-16T21:04:43Z

lib/src/component_declaration/redux_component.dart

+  set store(Store<V, B, A> value) => props[_storePropKey] = value;
+
+  /// The [ReduxActions] prop defined by [A].
+  A get actions => store.actions;


Can we add test coverage for this?

greglittlefield-wf · 2017-11-16T21:05:48Z

lib/src/component_declaration/redux_component.dart

+  @override
+  void componentWillReceiveProps(Map nextProps) {
+    super.componentWillReceiveProps(nextProps);
+    _tearDownSub();


Looks like this method is missing test coverage

greglittlefield-wf · 2017-11-16T21:05:53Z

lib/src/component_declaration/redux_component.dart

+  @mustCallSuper
+  @override
+  void componentWillUpdate(Map nextProps, Map nextState) {
+    if (_storeSub == null) _setUpSub(nextProps);


Looks like this method is missing test coverage

greglittlefield-wf · 2017-11-16T21:07:10Z

lib/src/component_declaration/redux_component.dart

+    Substate> implements UiComponent<T> {
+  Substate _connectedState;
+
+  /// The substate values of the redux store that this component component subscribes to.


typo: component component

greglittlefield-wf · 2017-11-16T21:07:37Z

lib/src/component_declaration/redux_component.dart

+
+  /// The substate values of the redux store that this component component subscribes to.
+  ///
+  /// It is reccommened to use this instead of `props.store.state`,


#nit comma should be a semicolon

greglittlefield-wf · 2017-11-16T21:13:18Z

lib/src/component_declaration/redux_component.dart

+  /// Related: [connect]
+  Substate get connectedState => _connectedState;
+
+  /// Subscribe to changes to the values from the redux store that this component cares about.


#nit This doesn't follow the Dart style guide convention around the first line of function doc comments.

Should be something like:

Returns a subset of the values derived from the redux store that this component cares about.

greglittlefield-wf · 2017-11-16T21:18:38Z

lib/src/component_declaration/redux_component.dart

+  @mustCallSuper
+  @override
+  void componentWillUpdate(Map nextProps, Map nextState) {
+    if (_storeSub == null) _setUpSub(nextProps);


Might be good to add a code comment here that explains when this is null (i.e., for component props updates and not just any rerender)

greglittlefield-wf · 2017-11-16T21:21:34Z

test/over_react/component_declaration/redux_component_test.dart

+      TestDefaultComponent component = getDartComponent(renderedInstance);
+
+      store.actions.trigger1();
+      await new Future.delayed(Duration.ZERO);


Is this necessary now that actions dispatch synchronously? Should we set the lower bound to the major version that changes that?

I wanted to leave it open to both version to not be blocking, I don't think w_comments is on that major version yet

👍

If you end up making changes in this PR again, perhaps a todo comment would be good to remove these if we ever bump. Waiting extra long shouldn't hurt, but it would be nice to verify that the things we expect to be synchronous are indeed synchronous.

greglittlefield-wf · 2017-11-16T21:24:11Z

test/over_react/component_declaration/redux_component_test.dart

+          reason: 'component should no longer be listening after unmount');
+    });
+
+    test('subscribes to any state changes subscribed to in connect', () async {


This description would be clearer and more indicative of what's being tested if it read "only the state changes subscribed to in connect".

greglittlefield-wf · 2017-11-16T21:25:11Z

test/over_react/component_declaration/redux_component_test.dart

+      expect(component.numberOfRedraws, 1);
+
+      component.props = TestDefault()..store = updatedStore;
+      component.redraw();


This should be actually rerendering the component and not just simulating a props update.

johnbland-wf · 2017-11-16T22:44:52Z

One more attempt for a rename here. ;)

ReduxUiComponent fits a redux.dart implementation. This is a forced BuiltRedux implementation.

I highly suggest renaming this so it fits the naming of Built*.

greglittlefield-wf · 2017-11-16T23:08:08Z

+1

…upport/dev # Conflicts: # pubspec.yaml

brianbolton-wf · 2017-11-29T16:04:21Z

FYI @christianplazas-wf @garywhitlock-wf

aaronlademann-wf

+1

aaronlademann-wf · 2017-11-30T14:16:00Z

+1 refresh

greglittlefield-wf · 2017-12-05T16:49:38Z

+1

jayudey-wf · 2017-12-06T17:07:16Z

QA +1

Testing instruction
Dev +1's
Dev/QA +10 with detail of what was tested
- passing build with tests exercising new functionality
Security review (if required)
Unit tests created/updated
All unit tests pass
Rosie has run and reports properly the release the ticket will be included in
Areas of regression noted

Merging into master.

Add ReduxUiComponent and ReduxUiStatefulComponent

e7c03a2

greglittlefield-wf reviewed Oct 3, 2017

View reviewed changes

Move built_value to a dev dependency

1068c46

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

aaronlademann-wf reviewed Oct 4, 2017

View reviewed changes

johnbland-wf reviewed Oct 4, 2017

View reviewed changes

davidmarne-wf reviewed Oct 4, 2017

View reviewed changes

evanweible-wf reviewed Oct 5, 2017

View reviewed changes

jacehensley-wf added 3 commits October 9, 2017 15:04

Use architecture suggestions given by @davidmarne-wf

4c3068b

Run generator during CI

43a51e1

Beef up documentation

20dc99d

johnbland-wf reviewed Oct 12, 2017

View reviewed changes

greglittlefield-wf requested changes Nov 16, 2017

View reviewed changes

jacehensley-wf added 2 commits November 16, 2017 15:41

Address CR feedback

cf31a16

Revert wrongly committed changes

4203d81

greglittlefield-wf approved these changes Nov 16, 2017

View reviewed changes

rmconsole-wf added the Merge Requirements Unmet label Nov 16, 2017

rmconsole-wf added the Merge Conflicts label Nov 18, 2017

Merge remote-tracking branch 'upstream/master' into add_built_redux_s…

b79e618

…upport/dev # Conflicts: # pubspec.yaml

rmconsole-wf removed the Merge Conflicts label Nov 20, 2017

aaronlademann-wf approved these changes Nov 29, 2017

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Nov 29, 2017

Prefix Redux classes with Built-

cbe800f

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Nov 29, 2017

jacehensley-wf changed the title ~~Add ReduxUiComponent and ReduxUiStatefulComponent~~ Add BuiltReduxUiComponent Nov 29, 2017

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Nov 30, 2017

Move BuiltReduxUiComponent to experimental library

2607fbc

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Dec 4, 2017

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Dec 5, 2017

jayudey-wf merged commit be116f1 into Workiva:master Dec 6, 2017

jacehensley-wf mentioned this pull request Dec 6, 2017

UIP-2806: Release over_react 1.19.0 #127

Merged

Add BuiltReduxUiComponent #118

Add BuiltReduxUiComponent #118

Conversation

jacehensley-wf commented Oct 3, 2017 • edited

Ultimate problem:

How it was fixed:

Testing suggestions:

Potential areas of regression:

aviary2-wf commented Oct 3, 2017 • edited

Raven

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

codecov-io commented Oct 3, 2017 • edited

Codecov Report

greglittlefield-wf commented Oct 3, 2017

jacehensley-wf commented Oct 3, 2017

greglittlefield-wf commented Oct 3, 2017

jacehensley-wf commented Oct 3, 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

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

johnbland-wf commented Oct 4, 2017

johnbland-wf commented Oct 4, 2017

jacehensley-wf commented Oct 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmarne-wf Oct 9, 2017 • edited

Choose a reason for hiding this comment

johnbland-wf commented Oct 4, 2017 • edited

greglittlefield-wf commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmarne-wf commented Oct 9, 2017 • edited by teresarevious-wf

jacehensley-wf commented Oct 9, 2017

johnbland-wf Oct 12, 2017 • edited

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

davidmarne-wf Oct 13, 2017 • edited

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

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

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

jacehensley-wf commented Oct 3, 2017 •

edited

aviary2-wf commented Oct 3, 2017 •

edited

codecov-io commented Oct 3, 2017 •

edited

davidmarne-wf Oct 9, 2017 •

edited

johnbland-wf commented Oct 4, 2017 •

edited

davidmarne-wf commented Oct 9, 2017 •

edited by teresarevious-wf

johnbland-wf Oct 12, 2017 •

edited

davidmarne-wf Oct 13, 2017 •

edited

johnbland-wf commented Nov 16, 2017 •

edited

jayudey-wf commented Dec 6, 2017 •

edited