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-1590: Deprecate the Required annotation (Includes UIP-1835) #51

Merged
Show file tree
Hide file tree
Changes from all commits
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
63 changes: 52 additions & 11 deletions lib/src/component_declaration/annotations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. #nit might be nice to have this message on the aliases as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

///
/// @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;
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh man this looks weird lol

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;
Expand Down
3 changes: 3 additions & 0 deletions lib/src/component_declaration/component_base.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -143,11 +144,13 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component {
}

@override
@mustCallSuper
void componentWillReceiveProps(Map newProps) {
validateRequiredProps(newProps);
}

@override
@mustCallSuper
void componentWillMount() {
validateRequiredProps(props);
}
Expand Down
53 changes: 46 additions & 7 deletions lib/src/transformer/impl_generation.dart
Original file line number Diff line number Diff line change
Expand Up @@ -399,13 +399,15 @@ class ImplGenerator {

String accessorName = variable.name.name;

T getConstantAnnotation<T>(AnnotatedNode member, String name, T value) {
return member.metadata.any((annotation) => annotation.name?.name == name) ? value : null;
}

annotations.Accessor accessorMeta = instantiateAnnotation(field, annotations.Accessor);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I don't think this will pick up the constants.

You'll probably have to just match them by name, like so:

T getConstantAnnotation<T>(AnnotatedNode member, String name, T value) =>
    member.metadata.any((annotation) => annotation.name?.name == name)) ? value : null;


annotations.Accessor requiredProp = instantiateAnnotation(field, 'requiredProp', annotations.requiredProp);
annotations.Accessor nullableRequiredProp = instantiateAnnotation(field, 'nullableRequiredProp', annotations.nullableRequiredProp);

if (requiredProp != null) {
  // ...
}
if (nullableRequiredProp != null) {
  // ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also probably emit a warning if they try to use @requiredProp/nullableRequiredProp/@Accessor together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I thought it would pick up on that sort of thing too. What happens if a consumers has there own const shorthand for some reason, should we be supporting that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we can't support that without performing a full analysis 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

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;

Expand All @@ -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 += ')';
Expand Down
1 change: 1 addition & 0 deletions pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
@@ -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<ComponentTestProps> ComponentTest;

@Props()
class ComponentTestProps extends UiProps {
@requiredProp
var required;

@nullableRequiredProp
var nullable;
}

@Component()
class ComponentTestComponent extends UiComponent<ComponentTestProps> {
@override
render() => Dom.div()();
}
Loading