Angular: Allow annotating required inputs

[MichaelRFairhurst]

Inputs and outputs are not required in angular, because while forgetting some can cause bugs, for others its OK. For instance, ngForTrackBy is completely optional.

But it would be nice if the analyzer could catch required inputs being missing. To do so and not report false errors, we would also have to know which inputs are not required (such as ngForTrackBy).

@Component(selector: 'my-name', template: '<b [style.color]="color">{{name}}</b>')
class MyNameComponent {
    @Input(optional: false)
    String name;

    @Input(optional: true)
    String color;

in template:

  <my-name [color]="'green'"></my-name><!-- forgot to specify [name]! -->

As for events, marking ngModelChange as required would catch the mistake of doing [ngModel]="x". We could easily have a warning ngModelChange not bound, did you use [ngModel] and mean to use [(ngModel)]? simply based on the suffix Change.

I've also seen a component that turns other components into buttons, using a (trigger) event. Marking trigger a required output means loading this button component and still using (click) is an error. Required events might be a bit strange, but if (event)="" is allowable then marking an event required can only do so much maximum damage.

Now the risk I see here is that I'm not sure this information would be very useful to the angular runtime. If that is the case, this might be confusing to users who aren't using the analysis plugin. If so, I could make an AngularAnalyzerHints repo with these annotations...though it probably won't get as much usage.

[matanlurey]

We could re-use the @required annotation from package:meta.

WDUT @pq or @bwilkerson?

[MichaelRFairhurst]

Ooh, that looks promising. Would be nice, though, if there were also an @optional annotation, so that we could create a hint when its missing: hint: using @optional or @required on input myInput would allow for better angular analysis

[bwilkerson]

I don't know what it would look like if you used @required, so I don't know what I think about it. Could you provide an example of what you're thinking of?

I will point out that @required reverses the default semantics for named parameters, which is, of course, that they are optional. At least in that realm there are only two options, so it seems unnecessary to require an @optional annotation to be used.

[MichaelRFairhurst]

It would look like so:

@Component(selector: 'my-comp', ...)
class MyAngularComponent {
  @required
  @Input
  String name;

  @Input
  String color;
}

Then it would be able to mark <my-comp></my-comp> and <my-comp [color]="color"></my-comp> as errors in the templates because [name] is not used, while still knowing that <my-comp [name]="name"></my-comp> is allowed even though [color] is not used.

The issue with not having two options is that there are really three:

1. input or output is required
2. input or output is not required
3. input or output was made before you could declare whether it was required or not

Now, its better if 3 is treated as 2 than 1, but ideally 3 is a mix of 2 and an error state; A hint saying to migrate it to 1 or 2. Also if 2. and 3. are the same, there is no discoverability that using @required will do anything.

[bwilkerson]

You're conflating "semantics of the input or output" with "backward compatibility" :-) Either a given input is required or it isn't. There's no "we don't know whether it's required because you didn't say one way or the other" state. Imagine if Dart didn't have a "final" keyword and we wanted to add one. It would be horrible to require all non-final variables to now be marked with a "mutable" keyword.

But as for re-using @required, as tempting as it is, I'm a bit leery of taking an annotation with semantics for Dart code and using it to represent semantics that are specific to a single framework, even if it is Angular. The reason I say that is that we would then need to build knowledge of Angular semantics into the analyzer, and I'd prefer to keep the two separate.

Currently analyzer ought to generate a diagnostic when @required is used somewhere other than on a named parameter, including on a field. (It doesn't, but it ought to.) But if we use it for this purpose then analyzer would need to know not to do so if the field is defined in a class annotated with the Angular @Component annotation.

[MichaelRFairhurst]

I see what you're saying about backward compatibility, note that I'm not talking about the behavior of @required on optional parameters. Simply on angular inputs & outputs. And agreed on the issue of validating its placement...best if the core analyzer doesn't do any special validation for angular, and yes, definitely would be best if it did still have its own validation. Bit of a pickle there.

In terms of what I'm trying to do, required is a poor default because not everyone uses static analysis (if non-intellij users shared code that didn't declare optionality it would work for them but not necessarily consumers of their library). Alternatively, optionality is a poor default because it is the exceptional case and undermines toolability. Probably the best option is to have no default...But given that all angular code specifies neither option, and not everyone will use our analyzer, we must have a default. Thus my thought of a third option which hints itself as incomplete and otherwise uses the unintrusive semantics of optionality.

Also just realized we have a problem with the 'inputs' and 'outputs' section of the @Component annotation...

This isn't a valid place for an annotation, is it?

@Component(
   ...
   inputs: [@required 'value'],
   outputs: [@required 'valueChange']
)
class BasicValueComponent extends TwoWayChangeValueBase {
  // empty
}

If we did decide to go down the separate-library-with-annotations path, we could create a new annotation for this case

@Component(...)
@InputsRequired(const ['value'])
@OutputsRequired(const ['valueChange'])
...

though here it would be pretty annoying if every input required double specification to make the "you didn't mark this field as optional or required for great profit" hint go away.

[TedSander]

I like this idea, but I would vote to only having an annotation for required but not optional. Otherwise I think it would be too verbose.

I would also prefer it be in the @Input annotation itself if possible as I like that semantics more. I guess more special syntax could be added to the map as we already have one if we want to rename the input. Yet this seems really ugly to support the map syntax.

We'll soon be able to prefer annotations always as the limitations will be gone. Maybe we only support this if you are using the annotation syntax and not the map syntax. Then decide if we want it in the current annotation, or a separate one.

[bwilkerson]

... note that I'm not talking about the behavior of @required on optional parameters.

Yes, I understood that. I was merely making an analogy.

Generally, when designing a language (and Angular is defining a language!) it is better to make the usual case the default and make it as terse as possible. There are exceptions, but they're rare. If required is the default and optional must be stated, then most existing code won't need to be updated. You do have a valid concern, though, about existing code that's never been checked and might not conform to the new default.

It was easier with @required because the default semantics is the semantics we had before we introduced the annotation. If you apply the same logic to your situation, then the default would be "unspecified" and you would presumably not generate any errors. Basically, you'd treat the annotations as optional information that will be checked if it's available (similar to the optional type information in Dart).

This isn't a valid place for an annotation, is it?

No, it isn't. You can only place annotations on a declaration.

[MichaelRFairhurst]

I would vote to only having an annotation for required but not optional. Otherwise I think it would be too verbose.

Definitely seeing this through the eyes of the analyzer guy, might very well be too verbose.

If its binary, for inputs I in general would prefer required as the default, and for outputs in general prefer optional as the default. Doing so would probably mean lots of extra errors when we roll it out, though they are nonfatal.

Maybe if we made sure to annotate all core shared components that we own before releasing, required by default could be a pretty unobtrusive release.

[TedSander]

As the angular_components TL I'd argue optional is the more common case or at least would provide nicer symantics and should be the default for inputs. Most components we try to have a reasonable default, and allow users to override it if needed. This makes the component easier to use, and less verbose.

This also has the benefit of allowing Components/Directives to opt-in to the system as it will work as it does today, and they can make their code easier for their users by adding the required annotation.

Probably Directives and internal angular components have mostly required inputs, but in practice from the widgets we write required is not the common case.

[MichaelRFairhurst]

Cool, that seems good. We're also not necessarily too stuck with whatever we pick. We can see if users wish the default were different or not (though I still fear a bit that people just won't get in the habit of marking anything as required)

[TedSander]

For angular_components we would probably implement this pretty quickly if it was available in tooling. I think it makes for a nicer user experience.

Hopefully other users will see the benefit from that system and start adding it too.

[MichaelRFairhurst]

Great!

So I guess this brings us back to the question of how it should be annotated.

There's a standalone library option that would look like this:

@Component(...)
@InputsRequired(const ['value'])
@OutputsRequired(const ['valueChange'])
class MyComponent {
  @Input()
  @Required()
  String myInput;
  ...

Or the part-of-angular option that would look like...this?

@Component(
  ...
  inputs: const [...],
  outputs: const [...],
  requiredInputs: const ['value'],
  requiredOutputs: const ['valueChange'])
class MyComponent {
  @Input(required: true)
  String myInput;
  ...

[TedSander]

I prefer the later, but it isn't my call.

If we want to we may be able to just support this with annotations if supporting the map is too difficult. Currently we can't use annotations everywhere, but that will be coming soon.

[zoechi]

@MichaelRFairhurst

ngModelChange as required would catch the mistake of doing [ngModel]="x"

I don't think [ngModel]="x" indicates a mistake.

For example

<my-custom [ngModel]="x" #self (click)="doSomethingOnValueChange(self.value)"

This might not be the most beautiful example but there are enough use cases where (ngModelChange)="..." doesn't work. For example because of timing issues.

[MichaelRFairhurst]

And I'm guessing that

   <my-custom [value]="x" #self (click)="doSomethingOnValueChange(self.value)"

isn't a viable workaround because of form integration from ngmodel?

This could be a workaround, though its ugly so it would have to be rare.

   <my-custom [ngModel]="x" (ngModelChange)=""

[zoechi]

I don't think there should be a workaround necessary for something that's valid and works fine.
But if they don't add @required to ngModelChange then the analyzer won't complain anyway.

[TedSander]

+1 I wouldn't expect a @required on ngModelChange there are very valid use cases without specifying it.

[matanlurey]

The work is unblocked to allow this, depending on the priority we could start sooner.

[matanlurey]

This hasn't gotten a lot of traction, so I'm closing for now.