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

UIP-2074 Dart2js warning workaround #52

Merged

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Mar 1, 2017

Ultimate problem:

There are dart2js warnings incorrectly emitted (likely due to dart-lang/sdk#16030) by each component class declared using over_react, of the form:

[Warning from Dart2JS on over_react|web/demos/button/index.dart]:
web/src/demo_components/button.dart:11:1:
'ButtonProps' doesn't implement 'abstract ReactElement call([dynamic a, dynamic a1, dynamic a2, dynamic a3, dynamic a4, dynamic a5, dynamic a6, dynamic a7, dynamic a8, dynamic a9, dynamic a10, dynamic a11, dynamic a12, dynamic a13, dynamic a14, dynamic a15, dynamic a16, dynamic a17, dynamic a18, dynamic a19, dynamic a20, dynamic a21, dynamic a22, dynamic a23, dynamic a24, dynamic a25, dynamic a26, dynamic a27, dynamic a28, dynamic a29, dynamic a30, dynamic a31, dynamic a32, dynamic a33, dynamic a34, dynamic a35, dynamic a36, dynamic a37, dynamic a38, dynamic a39])' declared in 'UiProps'.
Try adding an implementation of 'call' or declaring 'ButtonProps' to be 'abstract'.
class ButtonProps extends UiProps {    /* GENERATED CONSTANTS */ static const ConsumedProps $consumedProps = const ConsumedProps($props, $propKeys); static const PropDescriptor _$prop__skin = const PropDescriptor(_$key__skin), _$prop__size = const PropDescriptor(_$key__size), _$prop__isActive = const PropDescriptor(_$key__isActive), _$prop__isDisabled = const PropDescriptor(_$key__isDisabled), _$prop__isBlock = const PropDescriptor(_$key__isBlock), _$prop__href = const PropDescriptor(_$key__href), _$prop__target = const PropDescriptor(_$key__target), _$prop__type = const PropDescriptor(_$key__type); static const List<PropDescriptor> $props = const [_$prop__skin, _$prop__size, _$prop__isActive, _$prop__isDisabled, _$prop__isBlock, _$prop__href, _$prop__target, _$prop__type]; static const String _$key__skin = 'ButtonProps.skin', _$key__size = 'ButtonProps.size', _$key__isActive = 'ButtonProps.isActive', _$key__isDisabled = 'disabled', _$key__isBlock = 'ButtonProps.isBlock', _$key__href = 'href', _$key__target = 'target', _$key__type = 'ButtonProps.type'; static const List<String> $propKeys = const [_$key__skin, _$key__size, _$key__isActive, _$key__isDisabled, _$key__isBlock, _$key__href, _$key__target, _$key__type]; 
^^^^^
[Info from Dart2JS on over_react|web/demos/button/index.dart]:
packages/over_react/src/component_declaration/component_base.dart:359:3:
The method 'call' is declared here in class 'UiProps'.
  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]);
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

These logs are annoying and get in the way of legitimate dart2js warnings.

How it was fixed:

  • Work around the issue by:
    • adding an abstract declaration of call in each UiProps impl class
    • making the UiProps class abstract to support this (which we can do safely, since we already have a runtime error when it's instantiated)
  • Add a new test around generation

This method was found to work by chance after messing around with the generated output a bit.

Illustration of the changes in generated output:

-class FooProps extends UiProps {
+abstract class FooProps extends UiProps {
…
 }


 // -----------------------------------------------------------------------------
 //
 //   GENERATED IMPLEMENTATIONS

…

 // Concrete props implementation.
 //
 // Implements constructor and backing map, and links up to generated component factory.
 class _$ButtonPropsImpl extends FooProps {
…
+  // Work around https://github.com/dart-lang/sdk/issues/16030 by making
+  // the original props class abstract and redeclaring `call` in the impl class.
+  @override
+  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]);
 }

Results

Without these error messages, the pub build output for this repo went down from 54.3KB to 1.3KB.

For our large UI library, web_skin_dart, pub build output was reduced from 223.3KB to 11.9KB.

over_react - Before over_react - After
pub_build_before pub_build_now_trimmed

Testing suggestions:

  • Verify that existing tests pass (there is good integration test coverage around this area)
  • Verify that new test passes
  • pub serve and verify that the component demos render properly in dart2js

Potential areas of regression:

Component base code.


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

Do so by making the original props class abstract and redeclaring
`call` in the impl class.

We can safely make this abstract, since we already have a runtime
warning when it's instantiated.
@aviary2-wf
Copy link

Raven

Number of Findings: 0

@aaronlademann-wf
Copy link
Contributor

+1

NOICE

@jacehensley-wf
Copy link
Contributor

+1

@rmconsole3-wf rmconsole3-wf changed the title Dart2js warning workaround UIP-2074 Dart2js warning workaround Mar 1, 2017
@leviwith-wf
Copy link
Contributor

QA +1

  • logs are clean
  • test pass
  • components render as expected with dart2JS

  • Testing instruction
  • 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.

@leviwith-wf leviwith-wf merged commit b251f73 into Workiva:master Mar 2, 2017
greglittlefield-wf added a commit that referenced this pull request Jun 23, 2020
Add warning when href+target create a security vulnerability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants