[2.0.0-dev - dart2] AF-233 Consume react-dart callable class fixes by aaronlademann-wf · Pull Request #155 · Workiva/over_react

aaronlademann-wf · 2018-05-08T19:45:46Z

Depends on Workiva/react-dart#142

Problem(s):

In the Dart 2 SDK, the assignment of an object with a call method to a function type will now be treated as an implicit closurization of the .call method of the object. This is not compatible with how ReactComponentFactory works.
The MapBase additions from #151are no longer the best way to satisfy the missing concrete Map class impls.
Misc. housekeeping items need to be addressed to get the 2.0.0-dev branch ready to support further Dart 2 SDK testing / 2.0.0-dev.<n> releases as the Dart team moves closer and closer to a production release of the Dart 2 SDK.

Solution(s):

Consume the [5.0.0-dev - dart2] AF-222 Fix Dart 2 Callable Classes Issues react-dart#142 callable class fixes
The MapBase additions from AF-223 Begin preparations for Dart SDK 2 #151 were reverted and the new fields / getters / methods from that are present on the Map class in the Dart 2 SDK were instead implemented individually so we don't add a bunch of stuff from dart:collection that we don't actually need.
Address housekeeping items
1. Update Dart SDK dependency range to >=2.0.0-dev.0 <=2.0.0-dev.49.0
2. Address any lints / hints / errors found when the analysis server in Dart w.0.0-dev.49.0 runs

Testing Instructions:

Switch your Dart SDK to 2.0.0-dev.49.0
Serve the examples using the DDC:
```
pub serve --web-compiler=dartdevc
```
Load http://localhost:8080 and play around with the example components.
Verify that the examples work as expected without producing run time errors

Known Issues:

There are some issues present that, upon some investigation, appear to be unrelated to these changes:

There are numerous test failures centered around type casting. I am unsure if this is a test package issue, or if we need to do away with some of our "shared" test suites.

FYA: @greglittlefield-wf @clairesarsam-wf

FYI: @dustinlessard-wf @sebastianmalysa-wf @robbecker-wf

aviary3-wk · 2018-05-08T19:46:14Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on HipChat: InfoSec Forum.

aaronlademann-wf · 2018-05-10T14:13:09Z

@greglittlefield-wf I don't think we'll be able to get a passing travis build until https://github.com/cleandart/react-dart/pull/142/files is merged / released as 5.0.0-dev.alpha.0. Travis doesn't seem to like git dependencies

aaronlademann-wf · 2018-05-10T14:16:52Z

Also, once https://github.com/Workiva/smithy-runner-dart/pull/48 goes through we should be able to get the smithy build passing as well.

greglittlefield-wf

Looks good, mostly some comments about the map class stuff

greglittlefield-wf · 2018-05-10T17:25:23Z

lib/src/component_declaration/built_redux_component.dart

  @mustCallSuper
  @override
-  void redraw([callback()]) {
+  void redraw([Function callback]) {


This should be changed back (is this leftover from when it was typed to Function previously in react-dart?)

greglittlefield-wf · 2018-05-10T17:28:36Z

lib/src/component_declaration/component_base.dart

+/// > Necessary for eventual upgrade to the Dart 2 SDK.
+abstract class _OverReactMapBase<K, V> extends Object implements MapViewMixin<K, V>, Map<K, V> {
+  // Manually implement members from `Map` that appear in the Dart 2 SDK
+  // without having to extend from `MapBase`.


#nit I think this comment implies the wrong thing; we don't want to extend from MapBase because we want to just proxy that map.

Perhaps this class should be called _MapProxy or _OverReactMapView

greglittlefield-wf · 2018-05-10T17:29:10Z

lib/src/component_declaration/component_base.dart

+
+  // Manually implement members from `MapViewMixin`,
+  // since mixing that class in doesn't play well with the DDC.
+  // TODO find out root cause and reduced test case.


I think this block of comments is now outdated and can be removed, right?

greglittlefield-wf · 2018-05-10T17:31:19Z

lib/src/component_declaration/component_base.dart

 /// > Note: Implements [MapViewMixin] instead of extending it so that the abstract [Props] declarations
 /// don't need a constructor. The generated implementations can mix that functionality in.
-abstract class UiProps extends MapBase
+abstract class UiProps<K, V> extends _OverReactMapBase<K, V>


Is there a reason generic parameters were added to UiProps/UiState (and other places)? I'm concerned that that might introduce type complexity in props class inheritances (which can be problematic in dart2js; see dart-lang/sdk#14729) without providing much benefit.

greglittlefield-wf · 2018-05-10T17:32:51Z

lib/src/component_declaration/component_base.dart

-  @override remove(Object key) => _map.remove(key);
-  @override Iterable get values => _map.values;
-
  // Manually implement members from `StateMapViewMixin`,


(I think StateMapViewMixin is a typo and should say PropsMapViewMixin.)

Could we try mixing in `PropsMapViewMixin instead and see how it works with Dart 2's DDC?

greglittlefield-wf · 2018-05-10T17:37:51Z

test/over_react/component_declaration/component_base_test.dart

-      });
+      // TODO: The typing for `customFactory` is an analyzer error in Dart 2 (dev.49), is this DDC workaround + test still necessary or can it be removed?
+      //
+      // NOTE: This is no longer supportable in Dart 2 (should advertise as a breaking change in release notes)


Actually, can we just remove this test? We don't intend on doing anything with it, and we know this is going to break.

greglittlefield-wf · 2018-05-10T17:38:21Z

test/over_react/component_declaration/flux_component_test/default.dart


  @override
-  void setState(_, [callback()]) {
+  void setState(_, [Function callback]) {


This and its friends should be reverted as well.

greglittlefield-wf · 2018-05-10T17:41:29Z

test/over_react/shared/map_proxy_tests.dart

      when(mock.addAll(any)).thenReturn('value');

-      expect(proxy.addAll(testMap), isNull);
+      expect(() => proxy.addAll(testMap), isNull);


Huh, you can pass in a function and expect will evaluate it and match the result? Is this documented somewhere?

That I don't know for sure @greglittlefield-wf ... I did this to address a dart 2 analyzer error, and the test didn't fail. I didn't dig too much further into it.

In addition, these test are currently not run in the DDC because they us mirrors.

It looks like it doesn't work. This is causing failures in Dart 1:

00:16 +245 -20: [Chrome] test/over_react_component_declaration_test.dart: component base: UiState provides Map functionality: proxies the Map member: forEach [E] Expected: null Actual: <Closure: () => void from: () => proxy[$forEach](callback)> package:test expect over_react/shared/map_proxy_tests.dart 86:7 test.test.dart.fn

We'll have to find a different workaround for this issue; perhaps we just shouldn't assert the return value. Or just comment out the tests until we're running them in Dart 2.

@greglittlefield-wf this branch is targeting an SDK version of 2.0.0-dev.49.0, I wouldn't expect a lot of tests in this branch to pass using the dart 1 SDK.

greglittlefield-wf · 2018-05-10T17:41:56Z

test/over_react/shared/map_proxy_tests.dart

+      expect(() => proxy.forEach(callback), isNull);
      verify(mock.forEach(callback));
    });



We should add tests for the new Dart 2 Map members as well

@greglittlefield-wf agreed, but currently, these tests are skipped in the DDC because they make use of mirrors.

Yeah, at some point, we need to update these tests to use DDC-compatible mocks, or to not use mocks at all.

We do need some kind of test coverage around these methods; we should at least stub out these tests with FIXMEs or something so that we add them by the time we release these changes as stable.

greglittlefield-wf · 2018-05-10T17:42:16Z

test/over_react/util/handler_chain_util_test.dart

 import 'package:over_react/over_react.dart';
 import 'package:test/test.dart';

+typedef dynamic ChainFunction([_, __, ___, ____, _____, ______]);


I think this ended up being unused.

greglittlefield-wf

A couple things, but should be ready after this pass!

greglittlefield-wf · 2018-05-15T23:16:30Z

test/over_react/shared/map_proxy_tests.dart

      when(mock.addAll(any)).thenReturn('value');

-      expect(proxy.addAll(testMap), isNull);
+      expect(() => proxy.addAll(testMap), isNull);


It looks like it doesn't work. This is causing failures in Dart 1:

00:16 +245 -20: [Chrome] test/over_react_component_declaration_test.dart: component base: UiState provides Map functionality: proxies the Map member: forEach [E] Expected: null Actual: <Closure: () => void from: () => proxy[$forEach](callback)> package:test expect over_react/shared/map_proxy_tests.dart 86:7 test.test.dart.fn

We'll have to find a different workaround for this issue; perhaps we just shouldn't assert the return value. Or just comment out the tests until we're running them in Dart 2.

greglittlefield-wf · 2018-05-15T23:19:20Z

test/over_react/shared/map_proxy_tests.dart

+      expect(() => proxy.forEach(callback), isNull);
      verify(mock.forEach(callback));
    });



Yeah, at some point, we need to update these tests to use DDC-compatible mocks, or to not use mocks at all.

We do need some kind of test coverage around these methods; we should at least stub out these tests with FIXMEs or something so that we add them by the time we release these changes as stable.

greglittlefield-wf · 2018-05-15T23:50:47Z

lib/src/component_declaration/component_base.dart

    }
  }

-  // Manually implement members from `MapViewMixin`,


This branch looks so much better than what we currently have!

There are a few more things that could be cleaned up, though, and since they're hard to explain I've just made a PR into this branch: #158.

Clean up the rest of the Map grossness left over from DDC workarounds

aaronlademann-wf · 2018-05-17T17:03:37Z

@greglittlefield-wf I've merged in your changes, and your comments about the () => proxy thing not working in Dart 1 - I'm not sure they're really relevant since this is merging into the 2.0.0 branch which is a dart 2 SDK "working" branch.

I have tests passing locally - although there is likely still some work to be done getting them passing in Travis. I think this is a good first step though, and I'd like to get this merged unless there are major objections by you or @clairesarsam-wf.

greglittlefield-wf · 2018-05-17T21:47:01Z

@aaronlademann-wf I'd rather us just comment out the tests than check in invalid changes just to fix syntax errors.

We do need some kind of test coverage around these methods; we should at least stub out these tests with FIXMEs or something so that we add them by the time we release these changes as stable.

I also think we should stub those in so we don't forget about it.

…written

clairesarsam-wf · 2018-05-18T17:07:32Z

+10

Examples page renders as expected with no runtime errors

aaronlademann-wf added area-component_base State: dependent PR area-dependencies Type: tech debt labels May 8, 2018

aaronlademann-wf requested a review from a team as a code owner May 8, 2018 19:45

aaronlademann-wf force-pushed the dart2 branch from 766e914 to 3919ad5 Compare May 10, 2018 13:45

aaronlademann-wf mentioned this pull request May 10, 2018

Fix Dart SDK 2 typing issues Workiva/over_react_test#39

Merged

greglittlefield-wf requested changes May 10, 2018

View reviewed changes

aaronlademann-wf force-pushed the dart2 branch 2 times, most recently from 2eac0f7 to 492c653 Compare May 10, 2018 23:52

aaronlademann-wf and others added 14 commits May 14, 2018 09:48

Make over_react compatible with react-dart 5.0

a33868a

Make over_react compatible with dart 2

bc4fa73

Fix Dart SDK dependency bounds

0e1908e

Fix type soundness for running tests in dart 2

c0f6c56

Updat ref to react-dart branch

8b427cf

Use dart2-friendly version of built_redux

8c236ae

Update dart version for travis

c1700c0

testing new dart 2 image

2f20aa3

add git access

6451ee6

Try https instead of ssh for travis

5864ed1

Get dart 2 capable test package

dae0f6d

More dart2 pkg compatibility changes

4f195b0

use image from drydock-prod

e635160

Bump mockito minimum version

7dc7b11

aaronlademann-wf force-pushed the dart2 branch from a99c144 to c015993 Compare May 14, 2018 16:59

Address CR feedback

7694a3b

aaronlademann-wf force-pushed the dart2 branch from c015993 to 7694a3b Compare May 14, 2018 18:03

aaronlademann-wf added 3 commits May 14, 2018 11:10

Fix typo

a3534fc

Add dart2 deps to test/integrate pubspec

907e362

More travis build troubleshooting

e72812e

aaronlademann-wf force-pushed the dart2 branch from 4e2eb81 to e72812e Compare May 14, 2018 21:29

Clean up the rest of the Map grossness left over from DDC workarounds

472628a

greglittlefield-wf mentioned this pull request May 15, 2018

Clean up the rest of the Map grossness left over from DDC workarounds #158

Merged

greglittlefield-wf requested changes May 15, 2018

View reviewed changes

Merge pull request #158 from Workiva/dart2_ddc_map_cleanup

cf7c047

Clean up the rest of the Map grossness left over from DDC workarounds

Temporarily comment out broken tests / stub in tests that need to be …

b9519f6

…written

greglittlefield-wf approved these changes May 18, 2018

View reviewed changes

rmconsole-wf added the Merge Requirements Unmet label May 18, 2018

aaronlademann-wf merged commit c7d3d70 into 2.0.0-dev May 18, 2018

aaronlademann-wf added the dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility label Sep 7, 2018

aaronlademann-wf added this to the 2.0.0 milestone Sep 7, 2018

aaronlademann-wf deleted the dart2 branch September 7, 2018 23:26

rmconsole-readonly-wk mentioned this pull request Feb 8, 2019

CPLAT-246 CPLAT-245 Release over_react 2.0.0-alpha #239

Merged

Conversation

aaronlademann-wf commented May 8, 2018

Problem(s):

Solution(s):

Testing Instructions:

Known Issues:

Uh oh!

aviary3-wk commented May 8, 2018

Security Insights

Action Items

Uh oh!

aaronlademann-wf commented May 10, 2018

Uh oh!

aaronlademann-wf commented May 10, 2018

Uh oh!

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlademann-wf commented May 17, 2018

Uh oh!

greglittlefield-wf commented May 17, 2018

Uh oh!

clairesarsam-wf commented May 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants