diff --git a/lib/src/component/dom_components.dart b/lib/src/component/dom_components.dart index af7098721..c37da8c6d 100644 --- a/lib/src/component/dom_components.dart +++ b/lib/src/component/dom_components.dart @@ -57,9 +57,6 @@ class DomProps extends component_base.UiProps @override 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]); } // Include pieces from transformer_helpers so that consumers can type these instances @@ -86,9 +83,6 @@ class SvgProps extends component_base.UiProps @override 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]); } /// A class that provides namespacing for static DOM component factory methods, much like `React.DOM` in React JS. diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index 901bfe685..cdc379e28 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -638,31 +638,45 @@ abstract class UiProps extends MapBase /// /// Restricted statically to 40 arguments until the dart2js fix in /// is released. - 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]); - - /// Supports variadic children of the form `call([child1, child2, child3...])`. - @override - dynamic noSuchMethod(Invocation invocation) { - if (invocation.memberName == #call && invocation.isMethod) { - final positionalArguments = invocation.positionalArguments; - assert(_validateChildren(positionalArguments.length == 1 ? positionalArguments.single : positionalArguments)); - - final factory = componentFactory; - 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? - // ignore: avoid_as - return factory.build(props, invocation.positionalArguments); - } else { - var parameters = [] - ..add(props) - ..addAll(invocation.positionalArguments); - return Function.apply(factory, parameters); - } + /// + ReactElement call([c1 = _notSpecified, c2 = _notSpecified, c3 = _notSpecified, c4 = _notSpecified, c5 = _notSpecified, c6 = _notSpecified, c7 = _notSpecified, c8 = _notSpecified, c9 = _notSpecified, c10 = _notSpecified, c11 = _notSpecified, c12 = _notSpecified, c13 = _notSpecified, c14 = _notSpecified, c15 = _notSpecified, c16 = _notSpecified, c17 = _notSpecified, c18 = _notSpecified, c19 = _notSpecified, c20 = _notSpecified, c21 = _notSpecified, c22 = _notSpecified, c23 = _notSpecified, c24 = _notSpecified, c25 = _notSpecified, c26 = _notSpecified, c27 = _notSpecified, c28 = _notSpecified, c29 = _notSpecified, c30 = _notSpecified, c31 = _notSpecified, c32 = _notSpecified, c33 = _notSpecified, c34 = _notSpecified, c35 = _notSpecified, c36 = _notSpecified, c37 = _notSpecified, c38 = _notSpecified, c39 = _notSpecified, c40 = _notSpecified]) { + List childArguments; + // Use `identical` since it compiles down to `===` in dart2js instead of calling equality helper functions, + // and we don't want to allow any object overriding `operator==` to claim it's equal to `_notSpecified`. + if (identical(c1, _notSpecified)) { + childArguments = []; + } else if (identical(c2, _notSpecified)) { + childArguments = [c1]; + } else if (identical(c3, _notSpecified)) { + childArguments = [c1, c2]; + } else if (identical(c4, _notSpecified)) { + childArguments = [c1, c2, c3]; + } else if (identical(c5, _notSpecified)) { + childArguments = [c1, c2, c3, c4]; + } else if (identical(c6, _notSpecified)) { + childArguments = [c1, c2, c3, c4, c5]; + } else if (identical(c7, _notSpecified)) { + childArguments = [c1, c2, c3, c4, c5, c6]; + } else { + childArguments = [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] + .takeWhile((child) => !identical(child, _notSpecified)) + .toList(); } - return super.noSuchMethod(invocation); + assert(_validateChildren(childArguments.length == 1 ? childArguments.single : childArguments)); + + final factory = componentFactory; + 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? + return factory.build(props, childArguments); + } else { + var parameters = [] + ..add(props) + ..addAll(childArguments); + return Function.apply(factory, parameters); + } } /// Validates that no [children] are instances of [UiProps], and prints a helpful message for a better debugging @@ -906,3 +920,8 @@ class StateMeta implements AccessorMeta { const StateMeta({this.fields, this.keys}); } + +const _notSpecified = const NotSpecified(); +class NotSpecified { + const NotSpecified(); +} diff --git a/lib/src/transformer/impl_generation.dart b/lib/src/transformer/impl_generation.dart index 7f1a457ee..7294becd3 100644 --- a/lib/src/transformer/impl_generation.dart +++ b/lib/src/transformer/impl_generation.dart @@ -188,11 +188,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([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]);') ..writeln('}') ..writeln(); diff --git a/lib/src/util/react_util.dart b/lib/src/util/react_util.dart index 9909e1066..77cf0f41a 100644 --- a/lib/src/util/react_util.dart +++ b/lib/src/util/react_util.dart @@ -1,7 +1,7 @@ import 'dart:collection'; -import 'package:over_react/over_react.dart'; import 'package:over_react/component_base.dart' as component_base show UiProps; +import 'package:over_react/over_react.dart'; /// A `MapView` helper that stubs in unimplemented pieces of [UiProps]. /// @@ -69,5 +69,5 @@ class UiPropsMapView extends MapView throw new UnimplementedError('@PropsMixin instances do not implement componentFactory'); @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]) => throw new UnimplementedError('@PropsMixin instances do not implement call'); + 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]) => throw new UnimplementedError('@PropsMixin instances do not implement call'); } diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index 53154c594..413d004ba 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -30,56 +30,103 @@ import '../../test_util/test_util.dart'; import '../shared/map_proxy_tests.dart'; main() { - group('component base:', () { - group('UiProps', () { - group('warns and throws when rendering a DOM component', () { - bool warningsWereEnabled; - setUp(() { - warningsWereEnabled = ValidationUtil.WARNINGS_ENABLED; - ValidationUtil.WARNINGS_ENABLED = false; - startRecordingValidationWarnings(); - }); + void _commonNonInvokedBuilderTests(UiProps builder) { + bool warningsWereEnabled; + setUp(() { + warningsWereEnabled = ValidationUtil.WARNINGS_ENABLED; + ValidationUtil.WARNINGS_ENABLED = false; + startRecordingValidationWarnings(); + }); - tearDown(() { - ValidationUtil.WARNINGS_ENABLED = warningsWereEnabled; - stopRecordingValidationWarnings(); - }); + tearDown(() { + ValidationUtil.WARNINGS_ENABLED = warningsWereEnabled; + stopRecordingValidationWarnings(); + }); - test('when a single non-invoked builder child is passed in', () { - expect(() => Dom.div()(Dom.div()), throwsArgumentError); - verifyValidationWarning(contains('It looks like you are trying to use a non-invoked builder as a child.')); - }); + test('when a single non-invoked builder child is passed in', () { + 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(() => Dom.div()([ + test('when a list with a non-invoked builder child passed in', () { + expect(() => + builder([ Dom.div(), Dom.p()(), Dom.div() ]), throwsArgumentError); - verifyValidationWarning(contains('It looks like you are trying to use a non-invoked builder as a child.')); - }); + verifyValidationWarning(contains( + 'It looks like you are trying to use a non-invoked builder as a child.')); + }); - test('except when an iterable with a non-invoked builder child passed in', () { - var children = (() sync* { - yield Dom.div(); - yield Dom.p()(); - yield Dom.div(); - })(); - expect(() => Dom.div()(children), returnsNormally); - rejectValidationWarning(anything); - }); + test( + 'except when an iterable with a non-invoked builder child passed in', () { + var children = (() sync* { + yield Dom.div(); + yield Dom.p()(); + yield Dom.div(); + })(); + expect(() => builder(children), returnsNormally); + rejectValidationWarning(anything); + }); - test('when non-invoked builder children are passed in variadically via noSuchMethod', () { - expect(() => Dom.div()( - Dom.div(), - Dom.p()(), - Dom.div() + test('when non-invoked builder children are passed in variadically', () { + expect(() => + builder( + Dom.div(), + Dom.p()(), + Dom.div() ), throwsArgumentError); - verifyValidationWarning(contains('It looks like you are trying to use a non-invoked builder as a child.')); - }); + verifyValidationWarning(contains( + 'It looks like you are trying to use a non-invoked builder as a child.')); + }); + } + + 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 = builder(); + expect(getJsChildren(instance), isNull); + }); + + test('1', () { + final instance = builder(1); + expect(getJsChildren(instance), equals(1)); + }); + + const firstGeneralCaseVariadicChildCount = 2; + const maxSupportedVariadicChildCount = 40; + for (var i = firstGeneralCaseVariadicChildCount; i < maxSupportedVariadicChildCount; i++) { + final childrenCount = i; + test('$childrenCount', () { + final expectedChildren = new List.generate(childrenCount, (i) => i + 1); + final arguments = []..addAll(expectedChildren); + final instance = Function.apply(builder, arguments); + expect(getJsChildren(instance), expectedChildren); + }); + } + + test('$maxSupportedVariadicChildCount (and passes static analysis)', () { + 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)); + }); + }); + } + + group('component base:', () { + group('UiProps', () { + group('warns and throws when rendering a DOM component', () { + _commonNonInvokedBuilderTests(Dom.div()); }, testOn: '!js'); group('renders a DOM component with the correct children when', () { + _commonVariadicChildrenTests(Dom.div()); + test('no children are passed in', () { var renderedNode = renderAndGetDom(Dom.div()()); @@ -124,7 +171,7 @@ main() { expect(childNodes[1].text, equals('Second Child')); }); - test('children are set variadically via noSuchMethod', () { + test('children are set variadically', () { var firstChild = 'First Child'; var secondChild = 'Second Child'; var renderedNode = renderAndGetDom(Dom.div()(firstChild, secondChild)); @@ -137,54 +184,14 @@ main() { }); group('warns and throws when rendering a Dart composite component', () { - bool warningsWereEnabled; - setUp(() { - warningsWereEnabled = ValidationUtil.WARNINGS_ENABLED; - ValidationUtil.WARNINGS_ENABLED = false; - startRecordingValidationWarnings(); - }); - - tearDown(() { - ValidationUtil.WARNINGS_ENABLED = warningsWereEnabled; - stopRecordingValidationWarnings(); - }); - - test('when a single non-invoked builder child is passed in', () { - expect(() => TestComponent()(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(() => TestComponent()([ - Dom.div(), - Dom.p()(), - Dom.div() - ]), throwsArgumentError); - verifyValidationWarning(contains('It looks like you are trying to use a non-invoked builder as a child.')); - }); - - test('except when an iterable with a non-invoked builder passed in', () { - var children = (() sync* { - yield Dom.div(); - yield Dom.p()(); - yield Dom.div(); - })(); - expect(() => TestComponent()(children), returnsNormally); - rejectValidationWarning(anything); - }); - - test('when non-invoked builder children are passed in variadically via noSuchMethod', () { - expect(() => TestComponent()( - Dom.div(), - Dom.p()(), - Dom.div() - ), throwsArgumentError); - verifyValidationWarning(contains('It looks like you are trying to use a non-invoked builder as a child.')); - }); + _commonNonInvokedBuilderTests(Dom.div()); }, testOn: '!js'); group('renders a composite Dart component with the correct children when', () { + _commonVariadicChildrenTests(TestComponent()); + test('no children are passed in', () { + var renderedInstance = render(TestComponent()()); expect(getDartChildren(renderedInstance), new isInstanceOf(), reason: 'Should be a list because lists will be JSified'); @@ -237,7 +244,7 @@ main() { expect(getJsChildren(renderedInstance), orderedEquals(children)); }); - test('children are set variadically via noSuchMethod', () { + test('children are set variadically', () { var firstChild = 'First Child'; var secondChild = 'Second Child'; var renderedInstance = render(TestComponent()(firstChild, secondChild)); diff --git a/test/vm_tests/transformer/impl_generation_test.dart b/test/vm_tests/transformer/impl_generation_test.dart index 3b66a5f19..3d237b8b3 100644 --- a/test/vm_tests/transformer/impl_generation_test.dart +++ b/test/vm_tests/transformer/impl_generation_test.dart @@ -1052,40 +1052,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 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'); - - expect(propsImplCall.parameters.toSource(), uiPropsCall.parameters.toSource(), - 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', () {