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-2447 Make DomProps/SvgProps implement UiProps for more convenient DDC typing #87

Merged
merged 2 commits into from
Jun 26, 2017

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Jun 23, 2017

Ultimate problem:

The following code is affected by a DDC issue affecting emulated functions and noSuchMethod (issue: dart-lang/sdk#29904):

render() {
  var builder;
  if (condition) {
    builder = Dom.div();
  } else {
    builder = Button();
  }

  return builder(); // throws `Uncaught TypeError: Cannot read property 'noSuchMethod' of undefined`
}

The easiest way for consumers to work around it, is to type builder. In this case, however, the least upper bound is component_base.UiProps, so consumers would have to import that.

+import 'package:over_react/component_base.dart' as component_base;

 render() {
-  var builder;
+  component_base.UiProps builder;
   ...
 }

It would be nice if they could just do UiProps builder;, using the UiProps exposed by over_react.dart.

How it was fixed:

Make DomProps/SvgProps implement UiProps for more convenient typing in the scenario mentioned above.

Testing suggestions:

  • Verify all tests pass in content-shell and dart2js
  • Smoke-test in WSD docs.
  • Try executing the problematic code shown above in the DDC, and verify that it does not throw.

Potential areas of regression:

  • Dom components
  • DDC/dart2js runtime issues

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

@aviary2-wf
Copy link

Raven

Number of Findings: 0

@codecov-io
Copy link

Codecov Report

Merging #87 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #87   +/-   ##
=======================================
  Coverage   94.81%   94.81%           
=======================================
  Files          31       31           
  Lines        1522     1522           
=======================================
  Hits         1443     1443           
  Misses         79       79

@jacehensley-wf
Copy link
Contributor

+1

@clairesarsam-wf
Copy link
Contributor

+1

@rmconsole2-wf rmconsole2-wf changed the title Make DomProps/SvgProps implement UiProps for more convenient DDC typing UIP-2447 Make DomProps/SvgProps implement UiProps for more convenient DDC typing Jun 23, 2017
@aaronlademann-wf
Copy link
Contributor

@greglittlefield-wf when I add the following to web/index.dart:

void main() {
  // ...

  bool bar = false;
  
  ReactElement fooExample() {
    UiProps builder;
    if (bar) {
      builder = Dom.div();
    } else {
      builder = Button();
    }
  
    // ignore: invocation_of_non_function_expression
    return builder()('Herro');
  }

  // ...
}

I still see the following error when I load the page serving w/ the ddc...

Uncaught TypeError: obj.noSuchMethod is not a function
    at Object.dart.noSuchMethod (dart_sdk.js:2698)
    at callNSM (dart_sdk.js:2292)
    at Object.dart._checkAndCall (dart_sdk.js:2300)
    at Object.dart.dcall (dart_sdk.js:2332)
    at fooExample (index.dart:24)
    at Object.index.main (index.dart:27)
    at index.dart.bootstrap.js:125
    at Object.execCb (require.js:1696)
    at Module.check (require.js:878)
    at Module.<anonymous> (require.js:1139)

It also isn't a great story for the consumer - IMO - since the analyzer will tell them that they can't invoke a non-function expression... but I'm not sure of a way around that.

@greglittlefield-wf
Copy link
Contributor Author

@aaronlademann-wf You have an extra set of parentheses there, so that error is valid:

-return builder()('Herro');
+return builder('Herro');

@aaronlademann-wf
Copy link
Contributor

I feel so ashamed

@aaronlademann-wf
Copy link
Contributor

QA +10

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

@greglittlefield-wf
Copy link
Contributor Author

@aaronlademann-wf

I feel so ashamed

Haha don't worry, easy mistake to make.

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.

7 participants