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

AF-3731: Carry type parameters on generated empty prop/state mixins classes #220

Merged
merged 2 commits into from
Jan 7, 2019

Conversation

corwinsheahan-wf
Copy link
Contributor

Ultimate problem:

The transformer does not carry props/state mixins that have type parameters through to the generated $ prefixed class. Example:

@PropsMixin()
abstract class FooPropsMixin<T> {...}

// This is an analyzer error since $FooPropsMixin does not have any type parameters.
// We need the typing on $FooPropsMixin since, under Dart 2, the implemented getters 
// and setters for the mixin are in $FooPropsMixin and not FooPropsMixin.
class BarProps extends UiProps with FooPropsMixin<String>, $FooPropsMixin<String> {...}

// In the generated output:
abstract class $FooPropsMixin {...}

How it was fixed:

Carry type parameters from the declared props/state mixin class and add tests.

Testing suggestions:

  • CI Passes

Potential areas of regression:

Mixin generation


FYA: @greglittlefield-wf @aaronlademann-wf @kealjones-wk @evanweible-wf @sebastianmalysa-wf

@aviary3-wk
Copy link

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 Slack: #support-infosec.

var baz;
}
''');
List<T> bar;
Copy link
Contributor

Choose a reason for hiding this comment

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

T isn't actually defined in this example


test('with type parameters', () {
preservedLineNumbersTest('''
@PropsMixin() class FooPropsMixin<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be safe, let's use two params and have one of them use a bound as well to make sure that all works:

FooPropsMixin<T extends Iterable<T>, U>

@codecov-io
Copy link

Codecov Report

Merging #220 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master    #220   +/-   ##
======================================
  Coverage    90.1%   90.1%           
======================================
  Files          35      35           
  Lines        1767    1767           
======================================
  Hits         1592    1592           
  Misses        175     175

@evanweible-wf
Copy link
Contributor

QA +1

  • CI passes

  • Wrote a props mixin that uses a generic type param and a component that uses that mixin:

    import 'package:over_react/over_react.dart';
    
    // ignore: uri_has_not_been_generated
    part 'generic_props_mixin.over_react.g.dart';
    
    @Factory()
    UiFactory<FooProps> Foo =
        // ignore: undefined_identifier
        $Foo;
    
    @PropsMixin()
    abstract class ItemsPropsMixin<T extends Iterable> {
      // ignore: const_initialized_with_non_constant_value, undefined_class, undefined_identifier
      static const PropsMeta meta = $metaForItemsPropsMixin;
    
      Map get props;
    
      T items;
    }
    
    @Props()
    class _$FooProps extends UiProps
        with
            ItemsPropsMixin<Iterable<String>>,
            // ignore: mixin_of_non_class, undefined_class
            $ItemsPropsMixin<Iterable<String>> {}
    
    @Component()
    class FooComponent extends UiComponent<FooProps> {
      @override
      render() {
        return Dom.p()(props.items.join('\n'));
      }
    }
    // AF-3369 This will be removed once the transition to Dart 2 is complete.
    class FooProps extends _$FooProps
        with
            // ignore: mixin_of_non_class, undefined_class
            _$FooPropsAccessorsMixin {
      // ignore: const_initialized_with_non_constant_value, undefined_class, undefined_identifier
      static const PropsMeta meta = $metaForFooProps;
    }

    This code analyzes statically on Dart 1.24.3, provides the correct typing on props.items, and compiles to JS and renders without any errors.

@evanweible-wf
Copy link
Contributor

@Workiva/release-management-p

@rmconsole2-wf rmconsole2-wf merged commit eb1c1bf into master Jan 7, 2019
@rmconsole2-wf rmconsole2-wf deleted the AF-3731_carry_generics_on_mixins branch January 7, 2019 23:00
@evanweible-wf evanweible-wf added this to the Dart 2 Compat milestone Jan 7, 2019
@evanweible-wf evanweible-wf added the dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility label Jan 7, 2019
@evanweible-wf evanweible-wf modified the milestones: Dart 2 Compat, 2.0.0 (Dart 1 & 2 compatibility) Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility Merge Requirements Met RM Ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants