AF-3083: Transformer should emit an empty part file for libraries containing the .over_react.g.dart part directive #201

sebastianmalysa-wf · 2018-11-15T18:08:07Z

Proposed solution for https://jira.atl.workiva.net/browse/AF-3083

Ultimate problem:

The builder requires a part file, but the transformer does not. The updated boilerplate rollout will include a part directive that includes this new generated part, but that file will not exist when running in Dart 1.

To handle this, the transformer should output an empty file for each of these parts so that it does not fail at runtime.

How it was fixed:

find and remove the ignore comment and the associated part directive required by the builder

Testing suggestions:

Code changes and tests make sense
CI passes

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

aviary2-wf · 2018-11-15T18:08:27Z

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.

greglittlefield-wf · 2018-11-16T00:07:31Z

lib/transformer.dart

@@ -98,6 +98,26 @@ class WebSkinDartTransformer extends Transformer implements LazyTransformer {
    TransformedSourceFile transformedFile = new TransformedSourceFile(sourceFile);
    TransformLogger logger = new JetBrainsFriendlyLogger(transform.logger);

+    var ignoreCommentPattern = new RegExp(r'\/\/\s?ignore:\s?uri_does_not_exist,\s?uri_has_not_been_generated');
+    var partPattern = new RegExp(r'(\s*?)part\s(.*?).overReact.g.dart(.;)');


Not sure if this was already considered, but another option would be to emit an empty .g.dart file from the transformer, which is probably more robust than trying to do this transformation.

Just putting it out there.

We did talk about that and we went this direction because we didn't want to have to deal with mapping part files to their parent libraries, but I just realized we wouldn't have to do that since these .overReact.g.dart part files would only show up in the parent libraries.

So I think we should do that instead. Sorry I missed that before @sebastianmalysa-wf

…he ignore comment and remove it

evanweible-wf · 2018-11-19T16:15:23Z

test/vm_tests/transformer/impl_generation_test.dart

+
+      test('ignore line and part directive is removed from component file', () {
+        final ignoreLine = '\/\/ ignore: uri_does_not_exist, uri_has_not_been_generated';
+        final partLine = "part \'component\.overReact\.g\.dart\';";


Needs one more test to verify that the part directive is removed even if the ignore comment isn't found

evanweible-wf · 2018-11-19T16:23:49Z

lib/transformer.dart

@@ -98,6 +98,25 @@ class WebSkinDartTransformer extends Transformer implements LazyTransformer {
    TransformedSourceFile transformedFile = new TransformedSourceFile(sourceFile);
    TransformLogger logger = new JetBrainsFriendlyLogger(transform.logger);

+    var ignoreCommentPattern = new RegExp(r'\/\/\s?ignore:\s?uri_does_not_exist,\s?uri_has_not_been_generated');
+    var partPattern = new RegExp(r'(\s*?)part\s(.*?).overReact.g.dart(.;)');


I think this regex should actually enforce that the part directive starts at the very beginning of the line so that we avoid a commented out part directive. My suggestion would be:

var partPattern = new RegExp(r'''^part\s+['"].+.overReact.g.dart['"];''');

codecov-io · 2018-11-20T18:17:32Z

Codecov Report

Merging #201 into master will increase coverage by 1.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   94.54%   95.87%   +1.33%     
==========================================
  Files          34       34              
  Lines        1648     1669      +21     
==========================================
+ Hits         1558     1600      +42     
+ Misses         90       69      -21

sebastianmalysa-wf · 2018-11-20T19:45:43Z

Working on adding tests.

corwinsheahan-wf · 2018-11-28T21:00:16Z

@sebastianmalysa-wf to keep with dart naming conventions, we're gonna have the output extension be over_react.g.dart instead of overReact.g.dart. We'll have to update this PR to look for the former instead of the latter.

corwinsheahan-wf · 2018-11-30T20:43:42Z

test/vm_tests/transformer/test_data/componentWithPartDirective.dart

+// ignore: uri_does_not_exist, uri_has_not_been_generated
+part 'foo.over_react.g.dart';
+// ignore: uri_does_not_exist, uri_has_not_been_generated
+part 'bar.over_react.g.dart';


We'll only ever have 1 over_react.g.dart part directive in a file, so we should remove lines 5-6

… directive in a file

evanweible-wf · 2018-12-03T16:56:37Z

lib/transformer.dart

+    // For Dart 1 compatibility an empty generated part file will be created when a file contains
+    // the part directive pointing to the generated file the new builder requires.
+    if (partFilename != null) {
+      partPattern.allMatches(sourceFile.getText(0)).forEach((match) {


We only expect zero or one .over_react.g.dart part directives in any file, so I think instead of iterating over all matches we can just look for the first match.

evanweible-wf · 2018-12-03T16:57:25Z

test/vm_tests/transformer/test_data/componentWithPartDirective.dart

@@ -0,0 +1,15 @@
+import 'package:over_react/over_react.dart';


this filename should be component_with_part_directive.dart

evanweible-wf · 2018-12-03T16:59:48Z

test/vm_tests/transformer/transformer_test.dart

+      expect(fileAssets.length, equals(2));
+      expect(fileAssets[0].id.toString(), equals('testId|foo.over_react.g.dart'));
+      expect(fileAssets[1].id.toString(), equals('testId|componentWithPartDirective.dart'));
+      expect(fileAssets[0].toString(), equals('String ""'));


Instead of verifying the string representation of the asset, it'd be better to actually read the contents:

Suggested change

expect(fileAssets[0].toString(), equals('String ""'));

expect(fileAssets[0].readAsString(), isEmpty);

greglittlefield-wf · 2018-12-04T00:01:18Z

lib/transformer.dart

+    if (partFilename != null) {
+        var sourceFileDirectory = p.dirname(sourceFile.url.toFilePath());
+        var partFilePath = p.join(sourceFileDirectory, partFilename.group(1));
+        var asset = new Asset.fromString(new AssetId(transform.primaryInput.id.package, partFilePath), '');


Shouldn't this contain a part of directive?

Or, does it not matter since dart2js doesn't care? If so, fine by me 🤷‍♂️

Yeah it should, good catch.

var contents = "part of '${p.basename(sourceFile.url.toFilePath())}';"; var asset = new Asset.fromString(new AssetId(transform.primaryInput.id.package, partFilePath), contents);

@sebastianmalysa-wf seb we'll want to do this ^

greglittlefield-wf · 2018-12-04T00:08:39Z

test/vm_tests/transformer/transformer_test.dart

+      expect(initWithConfig({}).isPrimary(new AssetId(fakePackage, 'dart_test.yaml')), isFalse);
+    });
+
+    test('outputs an empty file when dart 2 boiler plate compatible part directive is found', () async {


Should we have a test for when an empty file should not be emitted?

…ut.id.path

greglittlefield-wf

+1

greglittlefield-wf · 2018-12-06T23:24:42Z

lib/transformer.dart

@@ -98,6 +98,21 @@ class WebSkinDartTransformer extends Transformer implements LazyTransformer {
    TransformedSourceFile transformedFile = new TransformedSourceFile(sourceFile);
    TransformLogger logger = new JetBrainsFriendlyLogger(transform.logger);

+    var partPattern = new RegExp(r'''part\s+['"](.+.over_react.g.dart)['"];''');


#supernit these periods aren't escaped in the regex

evanweible-wf · 2018-12-07T17:05:46Z

QA +1

CI passes

All tests passed in WSD branch with part directives added to every file with over_react code

+dependency_overrides:
+  over_react:
+    git:
+      url: git@github.com:sebastianmalysa-wf/over_react.git
+      ref: AF-3083

23:44 +27294: All tests passed!

@Workiva/release-management-p

AF-3083: Transformer should remove the .overReact.g.dart part directive

64ea320

sebastianmalysa-wf added the dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility label Nov 15, 2018

sebastianmalysa-wf requested a review from a team as a code owner November 15, 2018 18:08

rmconsole-wf added the Merge Requirements Unmet label Nov 15, 2018

AF-3083: remove extra spaces

85eefa5

greglittlefield-wf reviewed Nov 16, 2018

View reviewed changes

AF-3083: find the part directive and remove it first, then look for t…

3f0cebe

…he ignore comment and remove it

evanweible-wf reviewed Nov 19, 2018

View reviewed changes

sebastianmalysa-wf added 3 commits November 20, 2018 10:06

AF-3083: emit empty generated part file for Dart 1 builder compatibility

695a25d

AF-3083: use relative paths

6e931d0

AF-3083: clean up code and output an asset instead of creating a file

eb1a3a7

AF-3083: remove unused imports

5f7d170

sebastianmalysa-wf added the Hold Hold merges label Nov 20, 2018

sebastianmalysa-wf added 2 commits November 29, 2018 09:23

AF-3083: update partPattern to match the updated format

477d684

AF-3083: add tests

5ad9051

corwinsheahan-wf requested changes Nov 30, 2018

View reviewed changes

AF-3083: update test to reflect that there will ever only be one part…

8ca2473

… directive in a file

evanweible-wf requested changes Dec 3, 2018

View reviewed changes

AF-3083: address review comments

e7087e1

evanweible-wf approved these changes Dec 3, 2018

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Dec 3, 2018

corwinsheahan-wf approved these changes Dec 3, 2018

View reviewed changes

corwinsheahan-wf changed the title ~~AF-3083: Transformer should remove the .overReact.g.dart part directive~~ AF-3083: Transformer should remove the .over_react.g.dart part directive Dec 3, 2018

greglittlefield-wf reviewed Dec 4, 2018

View reviewed changes

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

evanweible-wf approved these changes Dec 5, 2018

View reviewed changes

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

corwinsheahan-wf changed the title ~~AF-3083: Transformer should remove the .over_react.g.dart part directive~~ AF-3083: Transformer should emit an empty part file for libraries containing the .over_react.g.dart part directive Dec 5, 2018

corwinsheahan-wf approved these changes Dec 5, 2018

View reviewed changes

sebastianmalysa-wf removed the Hold Hold merges label Dec 6, 2018

AF-3083: drop usage of toFilePath() in favour of transform.primaryInp…

9ded048

…ut.id.path

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Dec 6, 2018

corwinsheahan-wf approved these changes Dec 6, 2018

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Dec 6, 2018

evanweible-wf approved these changes Dec 6, 2018

View reviewed changes

greglittlefield-wf approved these changes Dec 7, 2018

View reviewed changes

AF-3083: escape periods in partPattern RegExp

ea6d623

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Dec 7, 2018

evanweible-wf approved these changes Dec 7, 2018

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Dec 7, 2018

corwinsheahan-wf approved these changes Dec 7, 2018

View reviewed changes

rmconsole-wf added the RM Ready label Dec 7, 2018

rmconsole2-wf merged commit b353c3e into Workiva:master Dec 7, 2018

rmconsole-readonly-wk mentioned this pull request Dec 20, 2018

AF-3002 AF-3001 Release over_react 1.30.0 #212

Merged

evanweible-wf added this to the Dart 2 Compat milestone Dec 21, 2018

evanweible-wf modified the milestones: Dart 2 Compat, 2.0.0 (Dart 1 & 2 compatibility) Jan 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AF-3083: Transformer should emit an empty part file for libraries containing the .over_react.g.dart part directive #201

AF-3083: Transformer should emit an empty part file for libraries containing the .over_react.g.dart part directive #201

sebastianmalysa-wf commented Nov 15, 2018 •

edited by evanweible-wf

Loading

aviary2-wf commented Nov 15, 2018 •

edited by aviary-wf

Loading

greglittlefield-wf Nov 16, 2018

evanweible-wf Nov 19, 2018

evanweible-wf Nov 19, 2018

evanweible-wf Nov 19, 2018

codecov-io commented Nov 20, 2018 •

edited

Loading

sebastianmalysa-wf commented Nov 20, 2018

corwinsheahan-wf commented Nov 28, 2018

corwinsheahan-wf Nov 30, 2018

evanweible-wf Dec 3, 2018

evanweible-wf Dec 3, 2018

evanweible-wf Dec 3, 2018

greglittlefield-wf Dec 4, 2018

evanweible-wf Dec 4, 2018

greglittlefield-wf Dec 4, 2018

greglittlefield-wf left a comment

greglittlefield-wf Dec 6, 2018

evanweible-wf commented Dec 7, 2018

		@@ -0,0 +1,15 @@
		import 'package:over_react/over_react.dart';

	expect(fileAssets[0].toString(), equals('String ""'));
	expect(fileAssets[0].readAsString(), isEmpty);

AF-3083: Transformer should emit an empty part file for libraries containing the .over_react.g.dart part directive #201

AF-3083: Transformer should emit an empty part file for libraries containing the .over_react.g.dart part directive #201

Conversation

sebastianmalysa-wf commented Nov 15, 2018 • edited by evanweible-wf Loading

Ultimate problem:

How it was fixed:

Testing suggestions:

aviary2-wf commented Nov 15, 2018 • edited by aviary-wf Loading

Security Insights

Action Items

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 Nov 20, 2018 • edited Loading

Codecov Report

sebastianmalysa-wf commented Nov 20, 2018

corwinsheahan-wf commented Nov 28, 2018

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

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanweible-wf commented Dec 7, 2018

sebastianmalysa-wf commented Nov 15, 2018 •

edited by evanweible-wf

Loading

aviary2-wf commented Nov 15, 2018 •

edited by aviary-wf

Loading

codecov-io commented Nov 20, 2018 •

edited

Loading