UIP-2410: Basic DDC support #82

greglittlefield-wf · 2017-06-20T22:03:46Z

Ultimate problem:

Running OverReact-based code in the DDC resulted in several runtime errors.

Also, the patterns of declaring abstract prop getters was problematic.

How it was fixed:

Fix DDC issues within internal code
- Major fixes:
  - Detect and patch DDC emulated function name bug in UiProps instances
    - This bug prevented UiProps classes from having props named name
  - Work around DDC emulated function bug encountered when invoking react-dart factories
    - Bump to react-dart ^3.4.0 to consume new .build() API
- Minor fixes:
  - Disable caching (for now) in getProps since Expandos not working on certain JS objects
  - Fix mixing in the same props class twice
  - Work around MapViewMixin and Props/StateMapViewMixin errors in DDC
Fix DDC issues likely to be experienced by consumers
- Add opt-in transformer flag (fixDdcAbstractAccessors) to work around DDC abstract accessor issue for props/state
  - See more info in transformer README changes
Add tests for:
- emulated function name bug workaround
  - Some DDC-specific have been added but are disabled for now, until we set up test to run in the DDC
- abstract accessor transformer workaround

Testing suggestions:

Verify all tests pass
- in Dartium and in the Dart VM: ddev test
- in dart2js: ddev test -p chrome (not done in CI, needs to be run manually)
Pull into WSD and verify that the docs build as expected in strong mode
- Use branch for this PR: https://github.com/Workiva/web_skin_dart/pull/763

Potential areas of regression:

UiProps + fluent interface
transformer

FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

…state

UIP-2410

aviary2-wf · 2017-06-20T22:03:50Z

Raven

Number of Findings: 0

aaronlademann-wf · 2017-06-20T22:27:05Z

lib/src/component/dom_components.dart

@@ -35,7 +35,7 @@ DomProps domProps([Map backingMap]) => new DomProps(null, backingMap);

 typedef DomProps DomPropsFactory();

-class DomProps extends component_base.UiProps with DomPropsMixin, ReactPropsMixin {
+class DomProps extends component_base.UiProps with DomPropsMixin {


Is this a breaking change?

I was thinking the same thing

This is part of "Fix mixing in the same props class twice"; ReactPropsMixin is already mixed in by component_base.UiProps

aaronlademann-wf · 2017-06-20T22:27:16Z

lib/src/component/dom_components.dart

@@ -45,7 +45,7 @@ class DomProps extends component_base.UiProps with DomPropsMixin, ReactPropsMixi
  final Map props;
 }

-class SvgProps extends component_base.UiProps with DomPropsMixin, ReactPropsMixin, SvgPropsMixin implements DomProps {
+class SvgProps extends component_base.UiProps with DomPropsMixin, SvgPropsMixin implements DomProps {


Is this a breaking change?

This is part of "Fix mixing in the same props class twice"; ReactPropsMixin is already mixed in by component_base.UiProps

aaronlademann-wf · 2017-06-20T22:28:26Z

lib/src/component_declaration/component_base.dart

+        var parameters = []
+          ..add(props)
+          ..addAll(invocation.positionalArguments);
+        return Function.apply(factory, parameters);


Does this need test coverage?

Good point; though 99% of cases will use a ReactComponentFactoryProxy, it's in here for backwards compatibility.

I'll add some tests around this.

Coverage added in 88bce6c

aaronlademann-wf · 2017-06-20T22:30:29Z

lib/src/transformer/impl_generation.dart

+            (member.isGetter || member.isSetter) &&
+            !member.isSynthetic &&
+            !member.isStatic &&
+            member.metadata.any((meta) => meta.name.name == 'override')


What happens if the consumer isn't annotating overrides?

Then they need to start annotating overrides if they want to get this opt-in fix 😛

I think that's fine, seeing as the @override restriction is documented. I put that restriction in place to avoid patching accessors that might not need it.

I'll let Trent know that we should require the annotate overrides lint then.

I'm not sure that's necessary. The issue that this fixes is quite apparent when building in the DDC, so it's easy to identify which getters are problematic. And then there's also the tool that @georgelesica-wf, which should also identify problematic getters.

Plus, I'm not sure that anyone else outside of our team even uses this pattern. .

Better safe than sorry

UIP-2410

codecov-io · 2017-06-20T23:57:06Z

Codecov Report

Merging #82 into master will decrease coverage by 2.77%.
The diff coverage is 93.19%.

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
- Coverage   97.58%   94.81%   -2.76%     
==========================================
  Files          29       31       +2     
  Lines        1402     1522     +120     
==========================================
+ Hits         1368     1443      +75     
- Misses         34       79      +45

UIP-2410

clairesarsam-wf

Not seeing any issues in the current state of this code - waiting on further test coverage to +1.

UIP-2410

aaronlademann-wf · 2017-06-21T01:28:13Z

@clairesarsam-wf ready for dev +1

clairesarsam-wf · 2017-06-21T16:02:58Z

+1

aaronlademann-wf · 2017-06-21T16:05:14Z

QA +10

Testing instruction
- Tests pass on content-shell running Dart 1.23.0
- Tests pass on chrome running Dart 1.23.0
Dev +1's
Dev/QA +10
Unit tests created/updated
All unit tests pass
Rosie ran/Rosie comment displays expected info
Dependency Scan Clean

Merging.

greglittlefield-wf added 11 commits June 20, 2017 14:51

Fix DDC issues with mixing in the same class twice

ceabacb

Work around MapViewMixin and Props/StateMapViewMixin errors in DDC

81da7a4

Work around DDC emulated function bug by using new react-dart .build

c485af5

Use transformer to work around DDC abstract accessor issue for props/…

0d671e2

…state

Disable props caching for DDC

ba6ee66

Detect and patch DDC emulated function name bug in UiProps instances

890b75f

UIP-2410

Only fix abstract accessors behind a flag

24b2c53

UIP-2410

Add tests for emulated function name bug workaround

48ae567

UIP-2410

Throw when fixDdcAbstractAccessors is specified but not a bool, tests

df5baef

UIP-2410

Add tests for abstract accessor fix, fix typing in generated getters

a22a0e9

UIP-2410

Bump to latest react-dart with new .build method now used by UiProps

cade984

UIP-2410

aaronlademann-wf added analyzer-strong-mode State: needs review labels Jun 20, 2017

aaronlademann-wf reviewed Jun 20, 2017

View reviewed changes

greglittlefield-wf added 2 commits June 20, 2017 16:05

Reenable getProps caching in non-DDC runtimes

db7d5d0

UIP-2410

Work around ddev coverage not respecting tags

b1a8bda

UIP-2410

greglittlefield-wf force-pushed the UIP-2410_ddc/dev branch from 6568d96 to b1a8bda Compare June 20, 2017 23:51

Add coverage for PropsMapViewMixin and StateMapViewMixin

8490f1e

UIP-2410

clairesarsam-wf reviewed Jun 21, 2017

View reviewed changes

Add test for calling non-ReactComponentFactoryProxy componentFactory

88bce6c

UIP-2410

rmconsole-wf added the Merge Requirements Met label Jun 21, 2017

aaronlademann-wf merged commit 86082d4 into Workiva:master Jun 21, 2017

aaronlademann-wf mentioned this pull request Jun 21, 2017

UIP-2406 Release over_react 1.12.0 #84

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIP-2410: Basic DDC support #82

UIP-2410: Basic DDC support #82

greglittlefield-wf commented Jun 20, 2017 •

edited

Loading

aviary2-wf commented Jun 20, 2017

aaronlademann-wf Jun 20, 2017

jacehensley-wf Jun 20, 2017

greglittlefield-wf Jun 20, 2017

aaronlademann-wf Jun 20, 2017

greglittlefield-wf Jun 20, 2017

aaronlademann-wf Jun 20, 2017

greglittlefield-wf Jun 20, 2017

greglittlefield-wf Jun 21, 2017 •

edited

Loading

aaronlademann-wf Jun 20, 2017

greglittlefield-wf Jun 20, 2017

greglittlefield-wf Jun 20, 2017

aaronlademann-wf Jun 20, 2017

greglittlefield-wf Jun 20, 2017

aaronlademann-wf Jun 20, 2017

codecov-io commented Jun 20, 2017 •

edited

Loading

clairesarsam-wf left a comment

aaronlademann-wf commented Jun 21, 2017

clairesarsam-wf commented Jun 21, 2017

aaronlademann-wf commented Jun 21, 2017

UIP-2410: Basic DDC support #82

UIP-2410: Basic DDC support #82

Conversation

greglittlefield-wf commented Jun 20, 2017 • edited Loading

Ultimate problem:

How it was fixed:

Testing suggestions:

Potential areas of regression:

aviary2-wf commented Jun 20, 2017

Raven

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 Jun 21, 2017 • edited Loading

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

codecov-io commented Jun 20, 2017 • edited Loading

Codecov Report

clairesarsam-wf left a comment

Choose a reason for hiding this comment

aaronlademann-wf commented Jun 21, 2017

clairesarsam-wf commented Jun 21, 2017

aaronlademann-wf commented Jun 21, 2017

greglittlefield-wf commented Jun 20, 2017 •

edited

Loading

greglittlefield-wf Jun 21, 2017 •

edited

Loading

codecov-io commented Jun 20, 2017 •

edited

Loading