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-3082: transform static meta fields #200

Merged
merged 3 commits into from
Nov 16, 2018

Conversation

sebastianmalysa-wf
Copy link
Contributor

Proposed solution to https://jira.atl.workiva.net/browse/AF-3082

Ultimate problem:

In anticipation of the Dart 2 transition, all props and state classes will have a static meta getter that is implemented to point to some symbol that needs to be generated, for example:

class FooProps {
  static const PropsMeta meta = $metaForFooProps;
  ...
}

The transformer will need to transform this meta static field inline:

class Foo Props{
  static const PropsMeta meta = $Props(FooProps);

How it was fixed:

  • identify static members with the meta variable name and replace the initializer to use the appropriate Props() or State() implementation within the source file .

Testing suggestions:

Code changes and tests make sense
CI passes


@corwinsheahan-wf @evanweible-wf @greglittlefield-wf

@sebastianmalysa-wf sebastianmalysa-wf requested a review from a team as a code owner November 13, 2018 21:30
@aviary-wf
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 HipChat: InfoSec Forum.

@codecov-io
Copy link

codecov-io commented Nov 13, 2018

Codecov Report

Merging #200 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   94.54%   94.57%   +0.03%     
==========================================
  Files          34       34              
  Lines        1648     1657       +9     
==========================================
+ Hits         1558     1567       +9     
  Misses         90       90

@@ -256,6 +256,55 @@ main() {
reason: 'should preserve existing inheritance');
});

test('with static PropsMeta declaration', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should also have a test which verifies this functionality on props/state mixins:

test('with static PropsMeta declaration in PropsMixin', () {
  final transformedLine = 'static const PropsMeta meta = \$Props(FooPropsMixin);';
  setUpAndGenerate('''
    @PropsMixin()
    class FooPropsMixin {
      static const PropsMeta meta = \$metaForFooPropsMixin;
    }
    '''
  );
  var transformedSource = transformedFile.getTransformedText();
  expect(transformedSource, contains(transformedLine));
});

);

var transformedSource = transformedFile.getTransformedText();
expect(transformedSource, contains(transformedLine));
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 it might make sense to also verify that the original meta line is not longer present to ensure that the transformer is replacing and not simply adding a line.

expect(transformedSource, contains(transformedLine));
});

test('with static StateMeta declaration', () {
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 we can combine the StateMeta and PropsMeta tests into 1 test, since any instance of a component will have at least a props class with a meta field.

.forEach((_field) {
final field = _field as FieldDeclaration; // ignore: avoid_as

annotations.Accessor accessorMeta = instantiateAnnotation(field, annotations.Accessor);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we skip lines 417-425? I don't think there should ever be an instance where a doNotGenerate() annotation is added to a meta field

…aration in PropsMixin and remove unnecessary check for doNotGenerate
@aaronlademann-wf aaronlademann-wf added the dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility label Nov 14, 2018
final transformedLine = 'static const PropsMeta meta = \$Props(FooPropsMixin);';

setUpAndGenerate('''
@PropsMixin()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get one more test added that tests this functionality on StateMixins?

@corwinsheahan-wf
Copy link
Contributor

QA +1

  • CI Passes
  • Added tests cover changes

Merging into master.
@Workiva/release-management-p

@rmconsole5-wk rmconsole5-wk merged commit 299e2c7 into Workiva:master Nov 16, 2018
@evanweible-wf evanweible-wf added this to the Dart 2 Compat milestone Dec 21, 2018
@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