diff --git a/lib/src/component_declaration/annotations.dart b/lib/src/component_declaration/annotations.dart index 9d1e2c17f..0ef4fba86 100644 --- a/lib/src/component_declaration/annotations.dart +++ b/lib/src/component_declaration/annotations.dart @@ -184,13 +184,48 @@ class StateMixin implements TypedMap { const StateMixin({this.keyNamespace: null}); } +/// Marks a `prop` as required to be set. +/// +/// Validation occurs in `UiComponent.validateRequiredProps` which requires super calls into `componentWillMount` and +/// `componentWillReceiveProps`. +/// +/// @Props() +/// abstract class FooProps { +/// @requiredProp +/// String requiredProp; +/// } +const Accessor requiredProp = const Accessor(isRequired: true); + +/// Marks a `prop` as required to be set, but allowed to be set explicitly to `null`. +/// +/// Validation occurs in `UiComponent.validateRequiredProps` which requires super calls into `componentWillMount` and +/// `componentWillReceiveProps`. +/// +/// @Props() +/// abstract class FooProps { +/// @nullableRequiredProp +/// String nullableRequiredProp; +/// } +const Accessor nullableRequiredProp = const Accessor(isRequired: true, isNullable: true); + /// Annotation used with the `over_react` transformer to customize individual accessors (props/state fields). /// +/// Validation occurs in `UiComponent.validateRequiredProps` which requires super calls into `componentWillMount` and +/// `componentWillReceiveProps`. +/// /// @Props() /// abstract class FooProps { /// @Accessor(keyNamespace: '', key: 'custom_key') /// String bar; +/// +/// @Accessor(isRequired: true) +/// String requiredProp; +/// +/// @Accessor(isRequired: true, isNullable: true) +/// String nullableRequiredProp; /// } +/// +/// Related: [requiredProp], [nullableRequiredProp]. class Accessor { /// A key for the annotated accessor, overriding the default of the accessor's name. final String key; @@ -199,22 +234,28 @@ class Accessor { /// overriding the default of `'${enclosingClassName}.'`. final String keyNamespace; + /// Whether the accessor is required to be set. + final bool isRequired; + + /// Whether setting a prop to null is allowed. + final bool isNullable; + + /// The error message displayed when the accessor is not set. + final String requiredErrorMessage; + const Accessor({ - this.key: null, - this.keyNamespace: null + this.key, + this.keyNamespace, + this.isRequired = false, + this.isNullable = false, + this.requiredErrorMessage, }); } -/// Annotation used with the `over_react` transformer to express a specific prop is required to be set. -/// -/// This is validated in `UiComponent.validateRequiredProps` which requires super calls into `componentWillMount` and -/// `componentWillReceiveProps`. +/// Deprecated. /// -/// @Props() -/// abstract class FooProps { -/// @Required() -/// String bar; -/// } +/// Use [Accessor], [requiredProp], or [nullableRequiredProp] instead. +@Deprecated('2.0.0') class Required { /// Whether setting a prop to null is allowed. final bool isNullable; diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index 62f4266ab..ca913acba 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -14,6 +14,7 @@ library over_react.component_declaration.component_base; +import 'package:meta/meta.dart'; import 'package:over_react/over_react.dart' show ClassNameBuilder, CssClassPropsMixin, @@ -143,11 +144,13 @@ abstract class UiComponent extends react.Component { } @override + @mustCallSuper void componentWillReceiveProps(Map newProps) { validateRequiredProps(newProps); } @override + @mustCallSuper void componentWillMount() { validateRequiredProps(props); } diff --git a/lib/src/transformer/impl_generation.dart b/lib/src/transformer/impl_generation.dart index e8994c204..b03806877 100644 --- a/lib/src/transformer/impl_generation.dart +++ b/lib/src/transformer/impl_generation.dart @@ -399,13 +399,15 @@ class ImplGenerator { String accessorName = variable.name.name; + T getConstantAnnotation(AnnotatedNode member, String name, T value) { + return member.metadata.any((annotation) => annotation.name?.name == name) ? value : null; + } + annotations.Accessor accessorMeta = instantiateAnnotation(field, annotations.Accessor); + annotations.Accessor requiredProp = getConstantAnnotation(field, 'requiredProp', annotations.requiredProp); + annotations.Accessor nullableRequiredProp = getConstantAnnotation(field, 'nullableRequiredProp', annotations.nullableRequiredProp); annotations.Required requiredMeta = instantiateAnnotation(field, annotations.Required); - bool isRequired = requiredMeta != null; - bool isNullable = isRequired && requiredMeta.isNullable; - bool hasErrorMessage = isRequired && requiredMeta.message != null && requiredMeta.message.isNotEmpty; - String individualKeyNamespace = accessorMeta?.keyNamespace ?? keyNamespace; String individualKey = accessorMeta?.key ?? accessorName; @@ -415,11 +417,48 @@ class ImplGenerator { String constantName = '${generatedPrefix}prop__$accessorName'; String constantValue = 'const $constConstructorName($keyConstantName'; - if (isRequired) { + var annotationCount = 0; + + if (accessorMeta != null) { + annotationCount++; + + if (accessorMeta.isRequired) { + constantValue += ', isRequired: true'; + + if (accessorMeta.isNullable) constantValue += ', isNullable: true'; + + if (accessorMeta.requiredErrorMessage != null && accessorMeta.requiredErrorMessage.isNotEmpty) { + constantValue += ', errorMessage: ${stringLiteral(accessorMeta.requiredErrorMessage)}'; + } + } + } + + if (requiredMeta != null) { constantValue += ', isRequired: true'; - if (isNullable) constantValue += ', isNullable: true'; - if (hasErrorMessage) constantValue += ', errorMessage: ${stringLiteral(requiredMeta.message)}'; + if (requiredMeta.isNullable) constantValue += ', isNullable: true'; + + if (requiredMeta.message != null && requiredMeta.message.isNotEmpty) { + constantValue += ', errorMessage: ${stringLiteral(requiredMeta.message)}'; + } + } + + if (requiredProp != null) { + annotationCount++; + constantValue += ', isRequired: true'; + } + + if (nullableRequiredProp != null) { + annotationCount++; + constantValue += ', isRequired: true, isNullable: true'; + } + + if (annotationCount > 1) { + logger.error( + '@requiredProp/@nullableProp/@Accessor cannot be used together.\n' + 'You can use `@Accessor(required: true)` or `isNullable: true` instead of the shorthand versions.', + span: getSpan(sourceFile, field) + ); } constantValue += ')'; diff --git a/pubspec.yaml b/pubspec.yaml index 25423441d..8c7de0f09 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -9,6 +9,7 @@ environment: dependencies: analyzer: ">=0.26.1+3 <0.30.0" barback: "^0.15.0" + meta: "^1.0.4" react: "^3.1.0" source_span: "^1.2.0" transformer_utils: "^0.1.1" diff --git a/test/over_react/component_declaration/transformer_integration_tests/constant_required_accessor_integration_test.dart b/test/over_react/component_declaration/transformer_integration_tests/constant_required_accessor_integration_test.dart new file mode 100644 index 000000000..90506a6ff --- /dev/null +++ b/test/over_react/component_declaration/transformer_integration_tests/constant_required_accessor_integration_test.dart @@ -0,0 +1,153 @@ +// Copyright 2016 Workiva Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +library over_react.component_declaration.transformer_integration_tests.constant_required_accessor_integration; + +import 'dart:html'; + +import 'package:over_react/over_react.dart'; +import 'package:react/react_dom.dart' as react_dom; +import 'package:test/test.dart'; + +import '../../../test_util/test_util.dart'; + +void main() { + group('properly identifies required props by', () { + group('throwing when a prop is required and not set', () { + test('on mount', () { + expect(() => render(ComponentTest()..nullable = true), + throwsPropError_Required('ComponentTestProps.required') + ); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect(() => react_dom.render((ComponentTest()..nullable = true)(), mountNode), + throwsPropError_Required('ComponentTestProps.required') + ); + }); + }); + + group('throwing when a prop is required and set to null', () { + test('on mount', () { + expect(() => render(ComponentTest() + ..required = null + ..nullable = true + ), throwsPropError_Required('ComponentTestProps.required')); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect( + () => react_dom.render((ComponentTest() + ..required = null + ..nullable = true + )(), mountNode), + throwsPropError_Required('ComponentTestProps.required') + ); + }); + }); + + group('throwing when a prop is nullable and not set', () { + test('on mount', () { + expect(() => render(ComponentTest()..required = true), + throwsPropError_Required('ComponentTestProps.nullable')); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect(() => react_dom.render((ComponentTest()..required = true)(), mountNode), + throwsPropError_Required('ComponentTestProps.nullable') + ); + }); + }); + + group('not throwing when a prop is required and set', () { + test('on mount', () { + expect(() => render(ComponentTest() + ..nullable = true + ..required = true + ), returnsNormally); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect(() => react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode), returnsNormally); + }); + }); + + group('not throwing when a prop is nullable and set to null', () { + test('on mount', () { + expect(() => render(ComponentTest() + ..nullable = null + ..required = true + ), returnsNormally); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect(() => react_dom.render((ComponentTest() + ..required = true + ..nullable = null + )(), mountNode), returnsNormally); + }); + }); + }); +} + +@Factory() +UiFactory ComponentTest; + +@Props() +class ComponentTestProps extends UiProps { + @requiredProp + var required; + + @nullableRequiredProp + var nullable; +} + +@Component() +class ComponentTestComponent extends UiComponent { + @override + render() => Dom.div()(); +} diff --git a/test/over_react/component_declaration/transformer_integration_tests/required_accessor_integration_test.dart b/test/over_react/component_declaration/transformer_integration_tests/required_accessor_integration_test.dart new file mode 100644 index 000000000..afcb26868 --- /dev/null +++ b/test/over_react/component_declaration/transformer_integration_tests/required_accessor_integration_test.dart @@ -0,0 +1,153 @@ +// Copyright 2016 Workiva Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +library over_react.component_declaration.transformer_integration_tests.required_accessor_integration; + +import 'dart:html'; + +import 'package:over_react/over_react.dart'; +import 'package:react/react_dom.dart' as react_dom; +import 'package:test/test.dart'; + +import '../../../test_util/test_util.dart'; + +void main() { + group('properly identifies required props by', () { + group('throwing when a prop is required and not set', () { + test('on mount', () { + expect(() => render(ComponentTest()..nullable = true), + throwsPropError_Required('ComponentTestProps.required', 'This Prop is Required for testing purposes.') + ); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect(() => react_dom.render((ComponentTest()..nullable = true)(), mountNode), + throwsPropError_Required('ComponentTestProps.required', 'This Prop is Required for testing purposes.') + ); + }); + }); + + group('throwing when a prop is required and set to null', () { + test('on mount', () { + expect(() => render(ComponentTest() + ..required = null + ..nullable = true + ), throwsPropError_Required('ComponentTestProps.required')); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect( + () => react_dom.render((ComponentTest() + ..required = null + ..nullable = true + )(), mountNode), + throwsPropError_Required('ComponentTestProps.required', 'This Prop is Required for testing purposes.') + ); + }); + }); + + group('throwing when a prop is nullable and not set', () { + test('on mount', () { + expect(() => render(ComponentTest()..required = true), + throwsPropError_Required('ComponentTestProps.nullable')); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect(() => react_dom.render((ComponentTest()..required = true)(), mountNode), + throwsPropError_Required('ComponentTestProps.nullable', 'This prop can be set to null!') + ); + }); + }); + + group('not throwing when a prop is required and set', () { + test('on mount', () { + expect(() => render(ComponentTest() + ..nullable = true + ..required = true + ), returnsNormally); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect(() => react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode), returnsNormally); + }); + }); + + group('not throwing when a prop is nullable and set to null', () { + test('on mount', () { + expect(() => render(ComponentTest() + ..nullable = null + ..required = true + ), returnsNormally); + }); + + test('on re-render', () { + var mountNode = new DivElement(); + react_dom.render((ComponentTest() + ..required = true + ..nullable = true + )(), mountNode); + + expect(() => react_dom.render((ComponentTest() + ..required = true + ..nullable = null + )(), mountNode), returnsNormally); + }); + }); + }); +} + +@Factory() +UiFactory ComponentTest; + +@Props() +class ComponentTestProps extends UiProps { + @Accessor(isRequired: true, requiredErrorMessage: 'This Prop is Required for testing purposes.') + var required; + + @Accessor(isRequired: true, isNullable: true, requiredErrorMessage: 'This prop can be set to null!') + var nullable; +} + +@Component() +class ComponentTestComponent extends UiComponent { + @override + render() => Dom.div()(); +} diff --git a/test/over_react/component_declaration/transformer_integration_tests/required_prop_integration_test.dart b/test/over_react/component_declaration/transformer_integration_tests/required_prop_integration_test.dart index b83f60bd5..231a07de3 100644 --- a/test/over_react/component_declaration/transformer_integration_tests/required_prop_integration_test.dart +++ b/test/over_react/component_declaration/transformer_integration_tests/required_prop_integration_test.dart @@ -26,20 +26,20 @@ void requiredPropsIntegrationTest() { group('properly identifies required props by', () { group('throwing when a prop is required and not set', () { test('on mount', () { - expect(() => render(ComponentTest()..nullableProp = true), - throwsPropError_Required('ComponentTestProps.requiredProp', 'This Prop is Required for testing purposes.') + expect(() => render(ComponentTest()..nullable = true), + throwsPropError_Required('ComponentTestProps.required', 'This Prop is Required for testing purposes.') ); }); test('on re-render', () { var mountNode = new DivElement(); react_dom.render((ComponentTest() - ..requiredProp = true - ..nullableProp = true + ..required = true + ..nullable = true )(), mountNode); - expect(() => react_dom.render((ComponentTest()..nullableProp = true)(), mountNode), - throwsPropError_Required('ComponentTestProps.requiredProp', 'This Prop is Required for testing purposes.') + expect(() => react_dom.render((ComponentTest()..nullable = true)(), mountNode), + throwsPropError_Required('ComponentTestProps.required', 'This Prop is Required for testing purposes.') ); }); }); @@ -47,43 +47,43 @@ void requiredPropsIntegrationTest() { group('throwing when a prop is required and set to null', () { test('on mount', () { expect(() => render(ComponentTest() - ..requiredProp = null - ..nullableProp = true - ), throwsPropError_Required('ComponentTestProps.requiredProp')); + ..required = null + ..nullable = true + ), throwsPropError_Required('ComponentTestProps.required')); }); test('on re-render', () { var mountNode = new DivElement(); react_dom.render((ComponentTest() - ..requiredProp = true - ..nullableProp = true + ..required = true + ..nullable = true )(), mountNode); expect( () => react_dom.render((ComponentTest() - ..requiredProp = null - ..nullableProp = true + ..required = null + ..nullable = true )(), mountNode), - throwsPropError_Required('ComponentTestProps.requiredProp', 'This Prop is Required for testing purposes.') + throwsPropError_Required('ComponentTestProps.required', 'This Prop is Required for testing purposes.') ); }); }); group('throwing when a prop is nullable and not set', () { test('on mount', () { - expect(() => render(ComponentTest()..requiredProp = true), - throwsPropError_Required('ComponentTestProps.nullableProp')); + expect(() => render(ComponentTest()..required = true), + throwsPropError_Required('ComponentTestProps.nullable')); }); test('on re-render', () { var mountNode = new DivElement(); react_dom.render((ComponentTest() - ..requiredProp = true - ..nullableProp = true + ..required = true + ..nullable = true )(), mountNode); - expect(() => react_dom.render((ComponentTest()..requiredProp = true)(), mountNode), - throwsPropError_Required('ComponentTestProps.nullableProp', 'This prop can be set to null!') + expect(() => react_dom.render((ComponentTest()..required = true)(), mountNode), + throwsPropError_Required('ComponentTestProps.nullable', 'This prop can be set to null!') ); }); }); @@ -91,21 +91,21 @@ void requiredPropsIntegrationTest() { group('not throwing when a prop is required and set', () { test('on mount', () { expect(() => render(ComponentTest() - ..nullableProp = true - ..requiredProp = true + ..nullable = true + ..required = true ), returnsNormally); }); test('on re-render', () { var mountNode = new DivElement(); react_dom.render((ComponentTest() - ..requiredProp = true - ..nullableProp = true + ..required = true + ..nullable = true )(), mountNode); expect(() => react_dom.render((ComponentTest() - ..requiredProp = true - ..nullableProp = true + ..required = true + ..nullable = true )(), mountNode), returnsNormally); }); }); @@ -113,21 +113,21 @@ void requiredPropsIntegrationTest() { group('not throwing when a prop is nullable and set to null', () { test('on mount', () { expect(() => render(ComponentTest() - ..nullableProp = null - ..requiredProp = true + ..nullable = null + ..required = true ), returnsNormally); }); test('on re-render', () { var mountNode = new DivElement(); react_dom.render((ComponentTest() - ..requiredProp = true - ..nullableProp = true + ..required = true + ..nullable = true )(), mountNode); expect(() => react_dom.render((ComponentTest() - ..requiredProp = true - ..nullableProp = null + ..required = true + ..nullable = null )(), mountNode), returnsNormally); }); }); @@ -139,11 +139,13 @@ UiFactory ComponentTest; @Props() class ComponentTestProps extends UiProps { + // ignore: deprecated_member_use @Required(message: 'This Prop is Required for testing purposes.') - var requiredProp; + var required; + // ignore: deprecated_member_use @Required(isNullable: true, message: 'This prop can be set to null!') - var nullableProp; + var nullable; } @Component() diff --git a/test/over_react_component_declaration_test.dart b/test/over_react_component_declaration_test.dart index 84c6e862e..668af3593 100644 --- a/test/over_react_component_declaration_test.dart +++ b/test/over_react_component_declaration_test.dart @@ -30,7 +30,9 @@ import 'over_react/component_declaration/transformer_helpers_test.dart' as trans import 'over_react/component_declaration/transformer_integration_tests/abstract_accessor_integration_test.dart' as abstract_accessor_integration_test; import 'over_react/component_declaration/transformer_integration_tests/accessor_mixin_integration_test.dart' as accessor_mixin_integration_test; import 'over_react/component_declaration/transformer_integration_tests/component_integration_test.dart' as component_integration_test; +import 'over_react/component_declaration/transformer_integration_tests/constant_required_accessor_integration_test.dart' as constant_required_accessor_integration_test; import 'over_react/component_declaration/transformer_integration_tests/namespaced_accessor_integration_test.dart' as namespaced_accessor_integration_test; +import 'over_react/component_declaration/transformer_integration_tests/required_accessor_integration_test.dart' as required_accessor_integration_test; import 'over_react/component_declaration/transformer_integration_tests/stateful_component_integration_test.dart' as stateful_component_integration_test; main() { @@ -45,6 +47,8 @@ main() { abstract_accessor_integration_test.main(); accessor_mixin_integration_test.main(); component_integration_test.main(); + constant_required_accessor_integration_test.main(); namespaced_accessor_integration_test.main(); + required_accessor_integration_test.main(); stateful_component_integration_test.main(); } diff --git a/test/vm_tests/transformer/impl_generation_test.dart b/test/vm_tests/transformer/impl_generation_test.dart index c86bb5cff..7670d6e5a 100644 --- a/test/vm_tests/transformer/impl_generation_test.dart +++ b/test/vm_tests/transformer/impl_generation_test.dart @@ -577,6 +577,47 @@ main() { '''); verify(logger.error('Fields are stubs for generated setters/getters and should not have initializers.', span: any)); }); + + group('accessors have', () { + const expectedAccessorErrorMessage = '@requiredProp/@nullableProp/@Accessor cannot be used together.\n' + 'You can use `@Accessor(required: true)` or `isNullable: true` instead of the shorthand versions.'; + + test('the Accessor and requiredProp annotation', () { + setUpAndGenerate(''' + @AbstractProps() + class AbstractFooProps { + @Accessor() + @requiredProp + var bar; + } + '''); + verify(logger.error(expectedAccessorErrorMessage, span: any)); + }); + + test('the Accessor and nullableRequiredProp annotation', () { + setUpAndGenerate(''' + @AbstractProps() + class AbstractFooProps { + @Accessor() + @nullableRequiredProp + var bar; + } + '''); + verify(logger.error(expectedAccessorErrorMessage, span: any)); + }); + + test('the requiredProp and nullableRequiredProp annotation', () { + setUpAndGenerate(''' + @AbstractProps() + class AbstractFooProps { + @requiredProp + @nullableRequiredProp + var bar; + } + '''); + verify(logger.error(expectedAccessorErrorMessage, span: any)); + }); + }); }); group('logs a warning when', () {