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-1637 First pass at strong mode compliance #15

Merged
merged 30 commits into from
Nov 2, 2016

Conversation

aaronlademann-wf
Copy link
Contributor

@aaronlademann-wf aaronlademann-wf commented Oct 28, 2016

Ultimate problem:
dart-lang/test#2331 - OverReact should be strong-mode compliant!

How it was fixed:

  1. Some changes were made to the react-dart wrapper.
  2. Lots of explicit type casting.
  3. Resolving misc. Dart analyzer warnings, errors and hints.
  4. For _FluxComponentMixin... I made it implement BatchedRedraws, but I'm not 100% confident in that solution. Would be great to get @maxwellpeterson-wf, @evanweible-wf or @trentgrover-wf to weight in on the validity of that.
  5. Turned on strong mode in .analysis_options.

Testing suggestions:

  1. Verify that all tests pass
  2. Verify that demo components behave as expected
  3. Try it out on some of our more complex internal Workiva components to verify that regressions are not being introduced.

Potential areas of regression:
Pretty much everything 😁


FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf @maxwellpeterson-wf, @evanweible-wf or @trentgrover-wf

@aviary-wf
Copy link

Raven

Number of Findings: 0

Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

Awesome, glad we're getting the ball rolling on this! Noticed a few things.


Side note: I'm not super familiar with best practices around writing strong-mode-compliant code, but at first glance, some of the typing in these changes seems to be a bit redundant.

Perhaps there are some patterns/tricks we can employ to make some of this code more concise. Of course, that can be done at a later time.

/// Renders a React component or builder into a detached node and returns the associtated Dart component.
react.Component renderAndGetComponent(dynamic component) => getDartComponent(render(component));
/// Renders a React component or builder into a detached node and returns the associated Dart component.
UiComponent renderAndGetComponent(dynamic component) => getDartComponent(render(component)) as UiComponent;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this changed to UiComponent? It's possible to use this to render a non-over_react component, so we should leave it as react.Component.

Copy link
Contributor Author

@aaronlademann-wf aaronlademann-wf Oct 28, 2016

Choose a reason for hiding this comment

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

@greglittlefield-wf I changed it because on line 335 of common_component_tests.dart, consumedProps is not available on Component.

I guess I can just do a cast to UiComponent in the test...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here 2804edf

/// If left unspecified one will be auto-generated for you to ensure
/// that the [caption] element is properly linked for accessibility purposes.
@override
String get id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the @virtual annotation be used on the super field to allow this? (See meta package 1.0.4 changelog)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greglittlefield-wf same as what I found in the react-dart PR... @virtual doesn't seem to make the error go away.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that only works when you override a field with a field 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually that doesn't even seem to work, at least on 1.19.1

@@ -287,7 +287,7 @@ bool _hasTestId(Map props, String key, String value) {

bool first = false;

var results = react_test_utils.findAllInRenderedTree(root, allowInterop((descendant) {
var results = react_test_utils.findAllInRenderedTree(root, allowInterop((Function descendant) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure descendant is always either ReactComponent or Element, not Function...

@codecov-io
Copy link

codecov-io commented Oct 28, 2016

Current coverage is 97.28% (diff: 100%)

Merging #15 into master will increase coverage by 0.25%

@@             master        dart-lang/test#2332   diff @@
==========================================
  Files            27         27          
  Lines          1211       1209     -2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1175       1176     +1   
+ Misses           36         33     -3   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 170a02e...1dbb92f

@aaronlademann-wf
Copy link
Contributor Author

@jacehensley-wf @greglittlefield-wf @clairesarsam-wf this is ready for another pass.

@@ -0,0 +1,436 @@
# Generated by pub
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be committed

@evanweible-wf
Copy link
Contributor

@aaronlademann-wf over_react's SDK range includes the latest (>=1.17.1), but the travis CI setup is pinned at 1.17.1. If you upgrade to the latest version of Dart, the analyzer step fails.

# Conflicts:
#	integrate/pubspec.yaml
+ Now that we’re gonna roll with a minimum SDK version of 1.19.1
@aaronlademann-wf
Copy link
Contributor Author

aaronlademann-wf commented Oct 31, 2016

@evanweible-wf @greglittlefield-wf I've merged in the contents of dart-lang/test#2340, which removes the need for some nasty type casting.

However - when I try to use bleeding edge sdk (1.20.1)... I get the following errors, all centered around FluxUiComponent stuff. Could one of you take a look there and see if there is a relatively painless "future-proofing" solution for that? I'm a little out-of-my-depths looking through how w_flux Store / Actions are getting typed within the transformer.

ERROR: Could not infer type parameter E, dynamic must be of type Store. ([over_react] lib/src/component_declaration/flux_component.dart:124)
ERROR: Could not infer type parameter E, dynamic must be of type Store. ([over_react] test/over_react/component_declaration/flux_component_test/handler_precedence.dart:18)
ERROR: Could not infer type parameter K, dynamic must be of type Store. ([over_react] test/over_react/component_declaration/flux_component_test/handler_precedence.dart:21)
ERROR: Could not infer type parameter E, dynamic must be of type Store. ([over_react] test/over_react/component_declaration/flux_component_test/redraw_on.dart:17)
ERROR: Could not infer type parameter K, dynamic must be of type Store. ([over_react] test/over_react/component_declaration/flux_component_test/store_handlers.dart:17)

@greglittlefield-wf
Copy link
Contributor

@evanweible-wf My junior year HS English teacher loved that word 😄.

+ The majority of the lints this uncovers must be cast at this time, but we should have the lint on to prevent any future unnecessary casts from being added to the library.
@aaronlademann-wf
Copy link
Contributor Author

ok... @evanweible-wf @greglittlefield-wf I think we're at a good place here. Final review maybe?

Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

A couple small things, mostly questions. Looks really good otherwise.

/// > Unsound implicit cast from ReactElement<dynamic> to ReactElement<Component>
void _reactElementTypingTest() {
///
/// Unsound implicit cast from ReactElement<dynamic> to ReactElement<Component>
Copy link
Contributor

Choose a reason for hiding this comment

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

ReactElement no longer has a generic parameter as of a breaking release or two ago, so we can probably just remove this whole file.

@@ -51,7 +51,9 @@ main() {
}

List<TestGenericType> generateBadTypeArgs() {
return new List.generate(arity, (_) => new Object());
// ignore: Type check failed
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I think this should just be List generateBadTypeArgs() {

@@ -158,7 +160,7 @@ main() {
test('calls all functions in order', () {
var calls = [];

var functions = new List.generate(5, (index) {
List<Function> functions = new List.generate(5, (index) {
Copy link
Contributor

Choose a reason for hiding this comment

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

#supernit for something like this, I think it might be preferred to do

var functions = new List<Function>.generate(5, (index) {

@evanweible-wf ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greglittlefield-wf @evanweible-wf when I do that, I get the following test failures:

00:11 +887 -1: [Dartium Content Shell] test/over_react_test.dart: HandlerChainUtil generic chaining: CallbackUtil0Arg chainFromList() returns a function of arity 0 that calls all functions in order
      type 'List<Function>' is not a subtype of type 'List<Callback0Arg>' of 'callbacks' where
        List is from dart:core
        Function is from dart:core
        List is from dart:core

      package:over_react/src/util/handler_chain_util.dart 212:41  CallbackUtil.chainFromList
      over_react/util/handler_chain_util_test.dart 167:42         main.<fn>.<fn>.sharedTests.<fn>.<fn>.<fn>

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, should it be this, then?

var functions = new List<Callback0Arg>.generate(5, (index) {

I'm fine leaving it as-is if it works, though

@@ -205,7 +205,7 @@ main() {
'correctly dispatches an event in resopnse to the first change', () async {
expect(querySelector('#rem_change_sensor'), isNull);

var calls = [];
List<double> calls = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

#supernit Same here: should this be

var calls = <double>[]?

Asking about these out mostly to improve my own knowledge.

@@ -175,7 +166,10 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component {
@override
TProps get props {
var unwrappedProps = this.unwrappedProps;
TProps typedProps = _typedPropsCache[unwrappedProps];
/// Have to cast as [TProps] until we can parameterize [_typedPropsCache].
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I wonder if we could reintroduce the type parameter on this Expando to avoid this cast... it seems the linked issue might only apply to mixins, so the omitted parameterization may just be carried over from when this code was part of a mixin.

These getters are used a lot, so it would be nice to avoid the perf hit of this cast if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just tried this locally and it works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greglittlefield-wf @jacehensley-wf Expandos parameterized, casts removed in c8b41b7

@@ -1,6 +1,6 @@
language: dart
dart:
- "1.17.1"
- "1.19.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two more spots where the analyzer is failing on 1.20.1, but they're easy fixes:

test/over_react/component_declaration/flux_component_test/handler_precedence.dart L21-25

  @override
  getStoreHandlers() => {props.store.store1: increment};

- increment(_) {
+ increment(Store store) {
    numberOfHandlerCalls += 1;
  }

Same thing for test/over_react/component_declaration/flux_component_test/store_handlers.dart L16-21.

With those two changes, I think you'd be fine to bump this up to 1.20.1

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if you're not using the variable is has to be typed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it has something to do with overriding the method but omitting the return type, I'm not sure.

List topLevelVarsOnly(String annotationName, Iterable<CompilationUnitMember> declarations) {
var topLevelVarDeclarations = [];
List<CompilationUnitMember> topLevelVarsOnly(String annotationName, Iterable<CompilationUnitMember> declarations) {
var topLevelVarDeclarations = <CompilationUnitMember>[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be <TopLevelVariableDeclaration>

List classesOnly(String annotationName, Iterable<CompilationUnitMember> declarations) {
var classDeclarations = [];
List<CompilationUnitMember> classesOnly(String annotationName, Iterable<CompilationUnitMember> declarations) {
var classDeclarations = <CompilationUnitMember>[];
Copy link
Contributor

Choose a reason for hiding this comment

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

..and these should be <ClassDeclaration> - that should fix the failing tests

@evanweible-wf
Copy link
Contributor

evanweible-wf commented Nov 1, 2016

@aaronlademann-wf @greglittlefield-wf I tried running tests on 1.20.1, but there were several failures that seem like they must be a bug. Same tests pass on 1.19.1:

      'http://localhost:58105/over_react/component/resize_sensor_test.dart': error: line 79 pos 51: expression is not a valid compile-time constant
              {ResizeSensorHandler onResize, int width: defaultContainerWidth, int height: defaultContainerHeight,
                                                        ^

      dart:async                                           Future.Future.microtask
      over_react/component/resize_sensor_test.dart         main.<fn>.expectResizeAfter
      over_react/component/resize_sensor_test.dart 239:32  main.<fn>.<fn>.<fn>.<async>
      ===== asynchronous gap ===========================
      dart:async                                           Future.Future.microtask
      over_react/component/resize_sensor_test.dart         main.<fn>.<fn>.<fn>

@greglittlefield-wf
Copy link
Contributor

greglittlefield-wf commented Nov 1, 2016

@evanweible-wf Looks like a bug indeed; I've got a reduced test case and am filing an SDK bug

Update: Filed this bug: dart-lang/sdk#27719

@aaronlademann-wf
Copy link
Contributor Author

@greglittlefield-wf @jacehensley-wf @evanweible-wf both dependent PRs have merged, react 3.0.1 has been published.

This should be ready for a final pass.

+ Since 99% of our contributors will never run pub get within that directory, and will therefore see a TON of errors when they first spin the project up.
Copy link
Contributor

@jacehensley-wf jacehensley-wf left a comment

Choose a reason for hiding this comment

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

Just a few really small things

@@ -364,19 +364,20 @@ class ImplGenerator {

typedMap.node.members
.where((member) => member is FieldDeclaration)
.where((FieldDeclaration member) => !member.isStatic)
.forEach((FieldDeclaration field) {
.where((member) => !(member as FieldDeclaration).isStatic) // ignore: avoid_as
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this and the previous line you could just do:

typedMap.node.members
    .where((memeber) => member is FieldDeclaration && !member.isStatic)
    .forEach(...

.where((FieldDeclaration member) => !member.isStatic)
.forEach((FieldDeclaration field) {
.where((member) => !(member as FieldDeclaration).isStatic) // ignore: avoid_as
.forEach((field) {
Copy link
Contributor

Choose a reason for hiding this comment

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

#nit Just so there is not a bunch of changes could the param be prefixed with an underscore and the new var not?

.forEach((_field) {
  final field = _field as FieldDeclaration;
})

@@ -294,7 +294,7 @@ main() {

group('modifyProps()', () {
test('passes the provided modifier itself', () {
modifier(UiProps props) {
modifier(Map<dynamic, dynamic> props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

#nit Unnecessary parameters

@aaronlademann-wf
Copy link
Contributor Author

@jacehensley-wf feedback addressed.

@@ -55,7 +55,9 @@ class ClassNameMatcher extends Matcher {
}

@override
bool matches(String className, Map matchState) {
bool matches(className, Map matchState) {
className = className as String; // ignore: avoid_as
Copy link
Contributor

Choose a reason for hiding this comment

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

We wouldn't have to do this if this were to be accepted https://github.com/dart-lang/matcher/issues/37

Copy link
Contributor

Choose a reason for hiding this comment

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

Update from that issue. On Dart 1.20.1 you can use @checked to override param types.

@greglittlefield-wf
Copy link
Contributor

#shipit

+1

@rm-astro-wf rm-astro-wf changed the title First pass at strong mode compliance UIP-1637 First pass at strong mode compliance Nov 2, 2016
@evanweible-wf
Copy link
Contributor

+1

@aaronlademann-wf
Copy link
Contributor Author

@leviwith-wf this is ready for QA / merge.

@leviwith-wf
Copy link
Contributor

QA Resource Approval: +10

  • tests pass (observed noted failures in current dart sdk)
  • provided examples behave themselves.

  • Testing instruction
  • Dev +1's
  • Dev/QA +10
  • Unit test created/updated
  • All unit tests pass

Merging.

@leviwith-wf leviwith-wf merged commit 6e6308e into Workiva:master Nov 2, 2016
greglittlefield-wf added a commit that referenced this pull request Jun 19, 2020
Clean up diagnostics code organization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants