AF-392: Add workaround to NSM for Dart 2 #207

corwinsheahan-wf · 2018-12-07T23:00:08Z

Ultimate problem:

In Dart 2, calls to noSuchMethod include the default values for positional parameters, and it is no longer possible to tell how many arguments have been specified: https://github.com/dart-lang/sdk/blob/master/CHANGELOG.md#200-dev560

This breaks our variadic children implementation, which uses noSuchMethod and the positional parameter length to know how many children were passed in.

This is a blocker for Dart 2.

How it was fixed:

Port changes/tests in react-dart involved in removing NSM from over_react
- https://github.com/cleandart/react-dart/pull/150/files#diff-73103ae4da65e607707d6ee86ee1d171R379
- https://github.com/cleandart/react-dart/pull/150/files#diff-7a382a75d4df367a3f5df34d837d9227R24

Testing suggestions:

Smoke test on dart 1 in web_skin_dart (tests pass, examples work, etc.)

Potential areas of regression:

React components

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

aviary3-wk · 2018-12-07T23:00:25Z

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.

lib/src/component_declaration/component_base.dart

greglittlefield-wf · 2018-12-07T23:59:10Z

lib/src/component/dom_components.dart

@@ -54,7 +54,7 @@ class DomProps extends component_base.UiProps
  String get propKeyNamespace => '';

  @override
-  ReactElement call([children, c2, c3, c4, c5, c6, c7, c8, c9, c10, c11, c12, c13, c14, c15, c16, c17, c18, c19, c20, c21, c22, c23, c24, c25, c26, c27, c28, c29, c30, c31, c32, c33, c34, c35, c36, c37, c38, c39, c40]);
+  ReactElement call([c1, c2, c3, c4, c5, c6, c7, c8, c9, c10, c11, c12, c13, c14, c15, c16, c17, c18, c19, c20, c21, c22, c23, c24, c25, c26, c27, c28, c29, c30, c31, c32, c33, c34, c35, c36, c37, c38, c39, c40]);


These can actually just get removed now, since they were put in as a workaround to not having NSM! a5c1820#diff-f41fd818c60cfc37d947e71aa885a019
🎉

lib/src/component_declaration/component_base.dart

test/over_react/component_declaration/component_base_test.dart

test/vm_tests/transformer/impl_generation_test.dart

lib/src/component_declaration/component_base.dart

* Naming adjustment * actually test paths for call() on UiProps * Remove unneeded call() instances

corwinsheahan-wf · 2018-12-13T17:00:00Z

lib/src/component_declaration/component_base.dart

+    if (factory is ReactComponentFactoryProxy) {
+      // Use `build` instead of using emulated function behavior to work around DDC issue
+      // https://github.com/dart-lang/sdk/issues/29904
+      // Should have the benefit of better performance; TODO optimize type check?


This TODO was already in the noSuchMethod implementation. Not sure if it should stay 🤷‍♂️

Hm, looking at this again, this type-check should be quite cheap, so I say we can remove this TODO

codecov-io · 2018-12-13T17:04:20Z

Codecov Report

Merging #207 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage    90.3%   90.31%   +0.02%     
==========================================
  Files          35       35              
  Lines        1772     1775       +3     
==========================================
+ Hits         1600     1603       +3     
  Misses        172      172

…hildren_dart2_workaround

corwinsheahan-wf · 2018-12-28T16:12:32Z

This is blocked until https://github.com/Workiva/graph_ui/pull/399 is merged.

…riadic_children_dart2_workaround

sebastianmalysa-wf · 2019-01-11T15:33:57Z

Consumed this in the following repos with no issues:

audit
datatables
graph_ui
home
text_doc_client
w_virtual_components
web_skin_dart

There were two errors found with this consumed in graph_app. The following PR fixes these errors and will need to be merged/released before this can move forward: https://github.com/Workiva/graph_app/pull/6723

sebastianmalysa-wf · 2019-01-24T15:10:54Z

Consumption PRs (checkmark indicates passing CI):

audit: https://github.com/Workiva/audit/pull/356
datatables: https://github.com/Workiva/datatables/pull/8160
graph_app: https://github.com/Workiva/graph_app/pull/6768
graph_ui: https://github.com/Workiva/graph_ui/pull/425
home: https://github.com/Workiva/home/pull/1649
text_doc_client: https://github.com/Workiva/text_doc_client/compare/consume_AF-392?expand=1
w_filing: https://github.com/Workiva/w_filing/pull/1005
w_virtual_components: https://github.com/Workiva/w_virtual_components/pull/335
web_skin_dart: https://github.com/Workiva/web_skin_dart/pull/1129

sebastianmalysa-wf · 2019-01-25T16:23:36Z

@corwinsheahan-wf @evanweible-wf web_skin_dart Skynet run is failing, but I don't think it's related to the consumption of this work. I think we're ready to move forward with this.

evanweible-wf · 2019-01-25T16:56:44Z

QA +1

CI passes
Consumer tests pass

@Workiva/release-management-p

Add workaround for NSM

d0a3cb7

corwinsheahan-wf requested a review from a team as a code owner December 7, 2018 23:00

rmconsole-wf added the Merge Requirements Unmet label Dec 7, 2018

corwinsheahan-wf changed the title ~~AF-392: Add workaround for NSM for Dart 2~~ WIP: AF-392: Add workaround for NSM for Dart 2 Dec 7, 2018

corwinsheahan-wf commented Dec 7, 2018

View reviewed changes

lib/src/component_declaration/component_base.dart Show resolved Hide resolved

corwinsheahan-wf commented Dec 7, 2018

View reviewed changes

lib/src/component_declaration/component_base.dart Outdated Show resolved Hide resolved

greglittlefield-wf reviewed Dec 8, 2018

View reviewed changes

corwinsheahan-wf added 2 commits December 10, 2018 11:56

Remove NSM

eeb947c

Address review comments

2e1cb13

* Naming adjustment * actually test paths for call() on UiProps * Remove unneeded call() instances

corwinsheahan-wf commented Dec 13, 2018

View reviewed changes

greglittlefield-wf approved these changes Dec 13, 2018

View reviewed changes

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

sebastianmalysa-wf approved these changes Dec 17, 2018

View reviewed changes

corwinsheahan-wf changed the title ~~WIP: AF-392: Add workaround for NSM for Dart 2~~ AF-392: Add workaround for NSM for Dart 2 Dec 18, 2018

corwinsheahan-wf changed the title ~~AF-392: Add workaround for NSM for Dart 2~~ AF-392: Add workaround to NSM for Dart 2 Dec 18, 2018

aaronlademann-wf approved these changes Dec 20, 2018

View reviewed changes

aaronlademann-wf added the dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility label Dec 20, 2018

evanweible-wf approved these changes Dec 20, 2018

View reviewed changes

rmconsole-wf added the Merge Conflicts label Dec 20, 2018

Merge remote-tracking branch 'upstream/master' into AF-392_variadic_c…

7f6be51

…hildren_dart2_workaround

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

evanweible-wf approved these changes Dec 20, 2018

View reviewed changes

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

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

Merge branch 'master' of github.com:Workiva/over_react into AF-392_va…

34fa0b8

…riadic_children_dart2_workaround

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Jan 10, 2019

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Jan 11, 2019

evanweible-wf approved these changes Jan 25, 2019

View reviewed changes

rmconsole-wf added the RM Ready label Jan 25, 2019

rmconsole7-wk merged commit cc52209 into master Jan 25, 2019

rmconsole7-wk deleted the AF-392_variadic_children_dart2_workaround branch January 25, 2019 16:57

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

rmconsole-readonly-wk mentioned this pull request Feb 8, 2019

CPLAT-3763 CPLAT-3762 Release over_react 1.31.0 #236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AF-392: Add workaround to NSM for Dart 2 #207

AF-392: Add workaround to NSM for Dart 2 #207

corwinsheahan-wf commented Dec 7, 2018 •

edited

Loading

aviary3-wk commented Dec 7, 2018

greglittlefield-wf Dec 7, 2018

corwinsheahan-wf Dec 13, 2018 •

edited

Loading

greglittlefield-wf Dec 13, 2018

codecov-io commented Dec 13, 2018 •

edited

Loading

corwinsheahan-wf commented Dec 28, 2018

sebastianmalysa-wf commented Jan 11, 2019 •

edited

Loading

sebastianmalysa-wf commented Jan 24, 2019 •

edited by evanweible-wf

Loading

sebastianmalysa-wf commented Jan 25, 2019

evanweible-wf commented Jan 25, 2019

AF-392: Add workaround to NSM for Dart 2 #207

AF-392: Add workaround to NSM for Dart 2 #207

Conversation

corwinsheahan-wf commented Dec 7, 2018 • edited Loading

Ultimate problem:

How it was fixed:

Testing suggestions:

Potential areas of regression:

aviary3-wk commented Dec 7, 2018

Security Insights

Action Items

greglittlefield-wf Dec 7, 2018

Choose a reason for hiding this comment

corwinsheahan-wf Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

greglittlefield-wf Dec 13, 2018

Choose a reason for hiding this comment

codecov-io commented Dec 13, 2018 • edited Loading

Codecov Report

corwinsheahan-wf commented Dec 28, 2018

sebastianmalysa-wf commented Jan 11, 2019 • edited Loading

sebastianmalysa-wf commented Jan 24, 2019 • edited by evanweible-wf Loading

sebastianmalysa-wf commented Jan 25, 2019

evanweible-wf commented Jan 25, 2019

corwinsheahan-wf commented Dec 7, 2018 •

edited

Loading

corwinsheahan-wf Dec 13, 2018 •

edited

Loading

codecov-io commented Dec 13, 2018 •

edited

Loading

sebastianmalysa-wf commented Jan 11, 2019 •

edited

Loading

sebastianmalysa-wf commented Jan 24, 2019 •

edited by evanweible-wf

Loading