UIP-1590: Deprecate the Required annotation (Includes UIP-1835) #51

jacehensley-wf · 2017-02-24T17:47:32Z

Ultimate problem:

The Required annotation used by the over_react transformer collides with the annotation from package:meta.

How it was fixed:

Deprecate Required, and add more options to the Accessor annotation.
Update logic around the transformer to support the new options on an Accessor annotation.
Added shorthand consts for Accessor(isRequired: true) and Accessor(isRequired: true, isNullable: true).

Testing suggestions:

Verify tests pass
On one of the example components, make a prop required via the new options on an Accessor annotation.
- Verify that when you don't set it, the component throws an error.

Potential areas of regression:

Required annotation support.

FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

UIP-1590

aviary2-wf · 2017-02-24T17:47:36Z

Raven

Number of Findings: 0

codecov-io · 2017-02-24T17:52:23Z

Codecov Report

Merging #51 into master will increase coverage by 0.03%.
The diff coverage is 63.16%.

@@            Coverage Diff            @@
##           master     #51      +/-   ##
=========================================
+ Coverage   97.67%   97.7%   +0.03%     
=========================================
  Files          28      28              
  Lines        1329    1344      +15     
=========================================
+ Hits         1298    1313      +15     
  Misses         31      31

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11188e2...6247b19. Read the comment docs.

aaronlademann-wf

Couple of nits... looks good otherwise.

aaronlademann-wf · 2017-02-24T18:14:51Z

lib/src/component_declaration/annotations.dart

+  /// Whether setting a prop to null is allowed.
+  final bool isNullable;
+
+  /// The erro message displayed when the accessor is not set.


#nit s/erro/error

aaronlademann-wf · 2017-02-24T18:15:47Z

lib/src/component_declaration/annotations.dart

-///       @Required()
-///       String bar;
-///     }
+/// Deprecated use [Accessor], [requiredProp], or [nullableRequiredProp] instead.


#nit

/// Deprecated. /// /// Use [Accessor], [requiredProp] or [nullableRequiredProp] instead.

UIP-1590

jacehensley-wf · 2017-02-24T20:18:38Z

@aaronlademann-wf Feedback has been addressed

aaronlademann-wf · 2017-02-24T21:22:56Z

+1

UIP-1590

greglittlefield-wf · 2017-02-24T22:54:09Z

lib/src/component_declaration/annotations.dart

 /// 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`.


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

greglittlefield-wf · 2017-02-24T22:54:46Z

lib/src/component_declaration/annotations.dart

+    this.key,
+    this.keyNamespace,
+    this.isRequired = false,
+    this.isNullable = false,


Oh man this looks weird lol

greglittlefield-wf · 2017-02-24T23:06:46Z

.../component_declaration/transformer_integration_tests/required_accessor_integration_test.dart

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


We should have integration tests for the alias/shorthand consts as well.

I actually don't think that they're getting picked up right now...

greglittlefield-wf · 2017-02-24T23:17:32Z

lib/src/transformer/impl_generation.dart

@@ -402,10 +402,6 @@ class ImplGenerator {
            annotations.Accessor accessorMeta = instantiateAnnotation(field, annotations.Accessor);


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) { // ... }

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

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?

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

UIP-1590

jacehensley-wf · 2017-02-27T17:50:07Z

@greglittlefield-wf Feedback has been addressed. The requiredProp and nullableRequiredProp are now actually being picked up by the transformer.

greglittlefield-wf · 2017-02-28T16:26:28Z

lib/src/transformer/impl_generation.dart

+            }
+
+            if (annotationCount > 1) {
+              logger.error('At most only a single annotation can be applied to an accessor.', span: getSpan(sourceFile, field));


This message might be a little misleading, since other annotations like @deprecated are also allowed.

What about something like:

@requiredProp/@nullableProp/@Accessor cannot be used together. You can use `@Accessor(required: true)` or `isNullable: true` instead of the shorthand versions.

UIP-1590

greglittlefield-wf · 2017-02-28T16:52:01Z

Code looks good
Tests pass in CI (Dart)
Tests pass locally in dart2js
Transformer fails gracefully w/ error message when multiple conflicting annotations are used

+10

leviwith-wf · 2017-02-28T22:18:44Z

QA +1

Merging.

[DIAGNOSTIC] hint for `consumedProps` returning a list literal

Deprecate Required, and add more options to Accessor.

c2bdea1

UIP-1590

jacehensley-wf added area-builder State: needs review Type: enhancement labels Feb 24, 2017

aaronlademann-wf reviewed Feb 24, 2017

View reviewed changes

Address CR feedback

e9d9b85

UIP-1590

jacehensley-wf added the Type: deprecation label Feb 24, 2017

aaronlademann-wf approved these changes Feb 24, 2017

View reviewed changes

rmconsole-wf added the Merge Requirements Met label Feb 24, 2017

Add mustCallSuper to componentWillReceiveProps and componentWillMount.

149a71b

UIP-1590

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Feb 24, 2017

Declare meta as a dependency

a1919db

UIP-1590

greglittlefield-wf reviewed Feb 24, 2017

View reviewed changes

Support the requiredProp and nullableRequiredProp annotations

b12cab0

UIP-1590

greglittlefield-wf reviewed Feb 28, 2017

View reviewed changes

Update error message when multiple accessor annotations are used.

6247b19

UIP-1590

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Feb 28, 2017

aaronlademann-wf added State: ready for QA and removed State: needs review labels Feb 28, 2017

leviwith-wf changed the title ~~UIP-1590: Deprecate the Required annotation~~ UIP-1590: Deprecate the Required annotation (Includes UIP-1835) Feb 28, 2017

leviwith-wf merged commit 8c403e5 into Workiva:master Feb 28, 2017

greglittlefield-wf mentioned this pull request Mar 3, 2017

UIP-2052 Release over_react 1.7.0 #53

Merged

greglittlefield-wf added a commit that referenced this pull request Jun 23, 2020

Merge pull request #51 from Workiva/link-2020/consumed-props-lints

cf0c462

[DIAGNOSTIC] hint for `consumedProps` returning a list literal

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIP-1590: Deprecate the Required annotation (Includes UIP-1835) #51

UIP-1590: Deprecate the Required annotation (Includes UIP-1835) #51

jacehensley-wf commented Feb 24, 2017

aviary2-wf commented Feb 24, 2017

codecov-io commented Feb 24, 2017 •

edited

Loading

aaronlademann-wf left a comment

aaronlademann-wf Feb 24, 2017

aaronlademann-wf Feb 24, 2017

jacehensley-wf commented Feb 24, 2017

aaronlademann-wf commented Feb 24, 2017

greglittlefield-wf Feb 24, 2017

jacehensley-wf Feb 27, 2017

greglittlefield-wf Feb 24, 2017

greglittlefield-wf Feb 24, 2017

jacehensley-wf Feb 27, 2017

greglittlefield-wf Feb 24, 2017

greglittlefield-wf Feb 24, 2017

jacehensley-wf Feb 27, 2017

greglittlefield-wf Feb 27, 2017

jacehensley-wf Feb 27, 2017

jacehensley-wf commented Feb 27, 2017

greglittlefield-wf Feb 28, 2017

greglittlefield-wf commented Feb 28, 2017

leviwith-wf commented Feb 28, 2017

		@@ -402,10 +402,6 @@ class ImplGenerator {
		annotations.Accessor accessorMeta = instantiateAnnotation(field, annotations.Accessor);

UIP-1590: Deprecate the Required annotation (Includes UIP-1835) #51

UIP-1590: Deprecate the Required annotation (Includes UIP-1835) #51

Conversation

jacehensley-wf commented Feb 24, 2017

Ultimate problem:

How it was fixed:

Testing suggestions:

Potential areas of regression:

aviary2-wf commented Feb 24, 2017

Raven

codecov-io commented Feb 24, 2017 • edited Loading

Codecov Report

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacehensley-wf commented Feb 24, 2017

aaronlademann-wf commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacehensley-wf commented Feb 27, 2017

Choose a reason for hiding this comment

greglittlefield-wf commented Feb 28, 2017

leviwith-wf commented Feb 28, 2017

codecov-io commented Feb 24, 2017 •

edited

Loading