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-2411: Basic DDC Support #121

Merged
merged 10 commits into from
Jun 20, 2017
Merged

Conversation

clairesarsam-wf
Copy link
Contributor

@clairesarsam-wf clairesarsam-wf commented Jun 19, 2017

Ultimate problem

Consumers need react-dart to be compilable with the ddc.

Solution

How to +10/QA

  • Verify tests pass locally in Chrome (dart2js) and content_shell
    • pub run test -p chrome
    • pub run test -p content-shell
  • Look at each of the test pages. None should have errors other than pre-existing key errors, expected key errors in call_and_nosuchmethod_test.html and an input related error in react_test.html.

please review: @greglittlefield-wf @jacehensley-wf @aaronlademann-wf

// customComponent({'key': 'twoVariadicChildren2'}, customComponent({}), customComponent({})),
// customComponent({'key': 'fiveVariadicChildren2'}, '', customComponent({}), '', customComponent({}), ''),
])
, querySelector('#content'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to ensure there are no regressions in variadic children in the VM or dart2js, can we perform these validations in actual test files?

Actually, I have some commits where there are tests wired up for passing in children... lemme see if I can get those changes in a good state for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Factory tests are now up on this PR. Should I remove this example file? It was a bit useful in poking around ddc runtime errors would be trivial to re-create for that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine leaving this example in

test('an empty List', () {
var instance = factory({}, []);
expect(getJsChildren(instance), equals([]));
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add one more test that a single iterable child is converted to a list successfully? (A regression test for your fix in 3dce4a5)

Copy link
Contributor Author

@clairesarsam-wf clairesarsam-wf Jun 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 664f842

@greglittlefield-wf
Copy link
Collaborator

+1

@aaronlademann-wf
Copy link
Collaborator

QA +10

  • Testing instruction
    • Verified all test pages have no new errors
  • Dev +1's
  • Unit tests created/updated
  • All unit tests pass
    • Tests pass on content-shell and chrome using Dart 1.23.0

@trentgrover-wf trentgrover-wf merged commit 95e59a6 into Workiva:master Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants