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

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

Merged
merged 5 commits into from
Jan 25, 2019
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address review comments
* Naming adjustment
* actually test paths for call() on UiProps
* Remove unneeded call() instances
corwinsheahan-wf committed Dec 13, 2018
commit 2e1cb13ef017fa6909eb51e83eaf0291fe62c624
6 changes: 0 additions & 6 deletions lib/src/component/dom_components.dart
Original file line number Diff line number Diff line change
@@ -52,9 +52,6 @@ class DomProps extends component_base.UiProps

@override
String get propKeyNamespace => '';

@override
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]);
}

// Include pieces from transformer_helpers so that consumers can type these instances
@@ -73,9 +70,6 @@ class SvgProps extends component_base.UiProps

@override
String get propKeyNamespace => '';

@override
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]);
}

/// A class that provides namespacing for static DOM component factory methods, much like `React.DOM` in React JS.
5 changes: 0 additions & 5 deletions lib/src/transformer/impl_generation.dart
Original file line number Diff line number Diff line change
@@ -193,11 +193,6 @@ class ImplGenerator {
..writeln(' /// The default namespace for the prop getters/setters generated for this class.')
..writeln(' @override')
..writeln(' String get propKeyNamespace => ${stringLiteral(propKeyNamespace)};')
..writeln()
..writeln(' // Work around https://github.com/dart-lang/sdk/issues/16030 by making')
..writeln(' // the original props class abstract and redeclaring `call` in the impl class.')
..writeln(' @override')
..writeln(' 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]);')
..writeln('}')
..writeln();

22 changes: 11 additions & 11 deletions test/over_react/component_declaration/component_base_test.dart
Original file line number Diff line number Diff line change
@@ -30,7 +30,7 @@ import '../../test_util/test_util.dart';
import '../shared/map_proxy_tests.dart';

main() {
void _commonNonInvokedBuilderTests(UiProps factory) {
void _commonNonInvokedBuilderTests(UiProps builder) {
bool warningsWereEnabled;
setUp(() {
warningsWereEnabled = ValidationUtil.WARNINGS_ENABLED;
@@ -44,14 +44,14 @@ main() {
});

test('when a single non-invoked builder child is passed in', () {
expect(() => factory(Dom.div()), throwsArgumentError);
expect(() => builder(Dom.div()), throwsArgumentError);
verifyValidationWarning(contains(
'It looks like you are trying to use a non-invoked builder as a child.'));
});

test('when a list with a non-invoked builder child passed in', () {
expect(() =>
factory([
builder([
Dom.div(),
Dom.p()(),
Dom.div()
@@ -67,13 +67,13 @@ main() {
yield Dom.p()();
yield Dom.div();
})();
expect(() => factory(children), returnsNormally);
expect(() => builder(children), returnsNormally);
rejectValidationWarning(anything);
});

test('when non-invoked builder children are passed in variadically', () {
expect(() =>
factory(
builder(
Dom.div(),
Dom.p()(),
Dom.div()
@@ -83,17 +83,17 @@ main() {
});
}

void _commonVariadicChildrenTests(UiProps factory) {
void _commonVariadicChildrenTests(UiProps builder) {
// There are different code paths for 0, 1, 2, 3, 4, 5, 6, and 6+ arguments.
// Test all of them.
group('a number of variadic children:', () {
test('0', () {
final instance = factory();
final instance = builder();
expect(getJsChildren(instance), isNull);
});

test('1', () {
final instance = factory(1);
final instance = builder(1);
expect(getJsChildren(instance), equals(1));
});

@@ -103,14 +103,14 @@ main() {
final childrenCount = i;
test('$childrenCount', () {
final expectedChildren = new List.generate(childrenCount, (i) => i + 1);
final arguments = <dynamic>[]..add(expectedChildren);
final instance = Function.apply(factory, arguments);
final arguments = <dynamic>[]..addAll(expectedChildren);
final instance = Function.apply(builder, arguments);
expect(getJsChildren(instance), expectedChildren);
});
}

test('$maxSupportedVariadicChildCount (and passes static analysis)', () {
final instance = factory(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40);
final instance = builder(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40);
// Generate these instead of hard coding them to ensure the arguments passed into this test match maxSupportedVariadicChildCount
final expectedChildren = new List.generate(maxSupportedVariadicChildCount, (i) => i + 1);
expect(getJsChildren(instance), equals(expectedChildren));
37 changes: 0 additions & 37 deletions test/vm_tests/transformer/impl_generation_test.dart
Original file line number Diff line number Diff line change
@@ -985,43 +985,6 @@ main() {
uiPropsCall = uiPropsClass.members
.singleWhere((entity) => entity is MethodDeclaration && entity.name?.name == 'call');
});

test('generates `call` override on the _\$*PropsImpl class correctly, as a workaround for dart-lang/sdk#16030', () {
setUpAndGenerate('''
@Factory()
UiFactory<FooProps> Foo;

@Props()
class FooProps {}

@Component()
class FooComponent {
render() => null;
}
''');

var transformedSource = transformedFile.getTransformedText();
var parsedTransformedSource = parseCompilationUnit(transformedSource);

ClassDeclaration propsImplClass = parsedTransformedSource.declarations
.singleWhere((entity) => entity is ClassDeclaration && entity.name?.name == r'_$FooPropsImpl');

MethodDeclaration propsImplCall = propsImplClass.members
.singleWhere((entity) => entity is MethodDeclaration && entity.name?.name == 'call');

var generatedParameterList = uiPropsCall.parameters.parameters.expand((param) => [param.identifier.name]);
var expectedParameterList = uiPropsCall.parameters.parameters.expand((param) => [param.identifier.name]);

expect(generatedParameterList.toString(), expectedParameterList.toString(),
reason: 'should have the correct number of arguments');
expect(propsImplCall.metadata, contains(predicate((meta) => meta.name?.name == 'override')),
reason: 'should have @override');
expect(propsImplCall.returnType, null,
reason: 'should not be typed, since ReactElement may not be imported');
expect(propsImplCall.isAbstract, isTrue,
reason: 'should be abstract; the declaration is only for dart2js bug workaround purposes, '
'and the inherited implementation should be used');
});
});

test('getComponentFactoryName() throws an error when its argument is null', () {