Angular: Expose "@visibleForTemplate" (for 2.1.0 SDK)

[matanlurey]

This is a "hotly requested" feature from large internal clients.

/// Declares a class member should only be accessed in the corresponding template.
///
/// Used to annotate a member (`M`) of a class (`C`) annotated with `@Component`.
/// Tools may provide feedback that accessing this field or method outside of the
/// context of the corresponding `template: ` or `templateUrl: ` HTML template is
/// invalid (i.e., the generated `<file>.template.dart` file given `<file>.dart`).
///
/// ```dart
/// import 'package:angular/angular.dart';
///
/// @Component(
///   selector: 'comp',
///   template: 'Hello {{name}}!',
/// )
/// class Comp {
///   @visibleForTemplate
///   String name = 'Eric';
/// }
/// ```
const Object visibleForTemplate = const _VisibleForTemplate();

class _VisibleForTemplate {
  const _VisibleForTemplate();
}

/cc @MichaelRFairhurst for thoughts as well.

[kevmoo]

Where would this annotation live so it could be useful as a lint?

package:meta is the most obvious, but it feels weird to put there 🤷‍♂️

[matanlurey]

I'd like it as part of package:angular, an idea like this was rejected from package:meta a while back, for fairly legitimate issues. It doesn't seem related enough, but we could talk to @bwilkerson again.

[alorenzen]

Could we do anything here in the angular compiler, or would this just apply the the analysis plugin?

[jonahwilliams]

How would this interact with unit tests? Would you still be allowed to access these members directly or would you have to query the DOM?

[matanlurey]

@alorenzen:

Could we do anything here in the angular compiler, or would this just apply the the analysis plugin?

Ideally both. My idea:

Stage 1

• [ ] Make it an analysis warning in the analyzer plugin
• [ ] Make it a compile-time warning in the compiler

Stage 2

• [ ] Make it an opt-in compile-time error

Stage 3

• [ ] Make it an opt-out compile-time error

We can probably stop here; internally we won't allow opt-outs after a point.

... having this as part of the analyzer proper will be easier than putting it directly in the compiler (and we could never block you using it dynamically, i.e. <dynamic>.foo, but I guess that's fine?

@jonahwilliams:

How would this interact with unit tests? Would you still be allowed to access these members directly or would you have to query the DOM?

Very good question. No idea, it would be reasonable to say its also visible to your own test/ package, but it's not clear if that would apply enough internally. Maybe we can just copy whatever @srawlins did for @visibleForTesting.

[alorenzen]

I'll phrase my question a little differently: How could we implement this in the compiler?

I personally think this is a great addition to the analysis plugin, but not sure it will actually be feasible in the compiler.

[matanlurey]

_How_ could we implement this in the compiler?

No idea, but unless we make the plugin fail builds I'm not sure how to enforce it.

[bwilkerson]

I generally think that annotations that are related to the Dart language in general belong in package:meta, but that annotations that are specific to a framework belong in the framework package. That has multiple advantages, and I can't think of a disadvantage.

[matanlurey]

@bwilkerson:

That has multiple advantages, and I can't think of a disadvantage.

One disadvantage is we don't have a great way to re-use code you already have for the other meta annotations in our own tools.

[jonahwilliams]

Couldn't this also be accomplished with more finely-grained visibility restrictions (besides public/package private). For example, some concept of friend libraries or similar to only expose the getter to the generated template.dart.file

[matanlurey]

@jonahwilliams:

Couldn't this also be accomplished with more finely-grained visibility restrictions

Yes, but I don't control that :)

[jonahwilliams]

Yes, but I don't control that :)

Of course! But if @visbleForTemplate is not feasible for Angular to implement, then it seems like a good feature request to have more visibility options. Then @visibleForTemplate would only have to slightly alter code generation.

[matanlurey]

@jonahwilliams:

good feature request

Yeah, ideally we'd have a more general:

@visibleForGeneratedCodeOrTesting

... or similar 😄 (friends, etc). But might take much longer.

/cc @lrhn @leafpetersen for FYI.

[bwilkerson]

One disadvantage is we don't have a great way to re-use code you already have for the other meta annotations in our own tools.

The code to check for invalid use of an annotation tends to be different for each annotation and fairly small, so we don't have a lot of sharing internally for this either. The quick fixes are a little more involved, but we've tried to make it very easy for plugins to add those.

That said, I'm more than happy to talk about your needs and whether we could add support to make it easier for you (or any other platform) to perform those checks.

[natebosch]

Seems like a big implementation cost for relatively low benefit. Most of the time people never see component instances anyway. If there are specific places where we want to expose a component instance it's better, IMO, to go the other way. Define an interface for your non-template interaction and provide the component instance as that interface. This is much safer (when we need the safety) than the other approach of selectively hiding bits of the API

[matanlurey]

@natebosch:

Most of the time people never see component instances anyway

Not true with APIs including injection and querying (@ViewChild) used a lot!

Define an interface for your non-template interaction and provide the component instance as that interface

Doesn't help with the @ViewChild concept :-/

[TedSander]

So there is another reason this would be really nice. For the M2 migration we need to keep API compatibility, but some of those APIs are just there for the template itself. It would be nice to mark them as such so that:
a) angular_components can have a contract to not support those items imperatively (This happens more than we would like on interfaces we would like to keep private, but can't thanks to the template)
b) For the M2 migration we can keep safely ignore those values to not implement because we know they will only be used in the local template, and not elsewhere.

[matanlurey]

Some changes to the above:

• We've agreed the compiler won't try to enforce this. It will be the analyzer, the plugin, or not at all.
• We've agreed a more _generic_ lint/warning is fine, but it needs to cover this use case at minimum.

[matanlurey]

The first PRs have gone in. We are waiting for SDK support on https://github.com/dart-lang/sdk/issues/33353 now.

[bencalabrese]

What is the implication for these getters being used in other templates by referencing a component with a template variable? Will that be discouraged or explicitly disallowed?

https://github.com/dart-lang/sdk/issues/33353#issuecomment-397767350 seems to indicate that the below would be valid.

@Component(
  selector: 'child',
  template: 'Look, it's {{seeminglyPrivate}}!',
)
class ChildComponent {
  @visibleForTemplate
  final seeminglyPrivate = 'secrets';
}

// Far away
@Component(
  selector: 'parent',
  template: '''
    <child #ref></child>
    I have access to your {{ref.seeminglyPrivate}}!
  '''
)
class ParentComponent {}

[matanlurey]

@bencalabrese:

What is the implication for these getters being used in other templates

Unfortunately, likely nothing - any file named *.template.dart would be allowed. This is a rarer occurrence (and usually its _setters_, not _getters_ that are more dangerous to access outside of the scope of the owning component). For example, the following code:

// a.dart
@Component(
  selector: 'a',
  template: '<b [privateThing]="'Hello'"></b>',
  directives: [B],
)
class A {}

// b.dart
@Component(
  selector: 'b',
  template: 'Hello {{b}}!',
)
class B {
  @visibleForTemplate
  @Input('privateThing')
  String b;
}

... would need the following code generated in a.template.dart:

_childCompB.b = 'Hello';

We _could_ limit @visibleForTemplate and say it _only_ applies to private state of the specific component - but that also means you could not disallow imperative access of @Input(), @Output(), or setters/getters that are for @ViewChildren() (and related) only. It would also be tricky - what if there are _multiple_ components in a single library.dart.

We could also introduce _multiple_ annotations, i.e.:

• @visibleForTemplate: Visible to your _own_ template, only.
• @visibleForAnyTemplate: Visible to _any_ *.template.dart files.

I'm not sure this is worth its weight considering it is a static check only. For example, you could do:

class ParentComp {
  @visibleForTemplate
  String doNotTouch;
}

class ChildComp {
  ChildComp(ParentComp p) {
    dynamic d = p;
    d.doNotTouch = 'Hehe'!;
  }
}

(And this technique works for _anything_, i.e. @protected, @visibleForTesting).

We _also_ could try to have the AngularDart Analyzer Plugin lint usages in #refs

/cc @nshahan @TedSander @MichaelRFairhurst on thoughts here.

[matanlurey]

@zoechi: Please do not suggest the behavior of something without knowing what the answer is - in this case you were not correct. It is OK to have opinions, but it can be very confusing for readers and users if they read your message, and think it is correct, and then we have to back-track when that is not the case.

[bencalabrese]

Thanks for clarifying, @matanlurey.

I agree: these cases are much less common. Maybe this case would be served well enough by additions to the style guide describing what should be considered part of a components public API vs what is expected to be private to the component's template?

We're starting to get into more complicated territory, but regarding > 1 component per dart library, could something along the lines of: @VisibleForTemplate('foo.html') work? That doesn't handle the case of inline templates, but maybe there's a solution for that?

Part of me is inclined to say that if someone is casting to dynamic they're taking responsibility for whatever happens thereafter. That might be too punitive though, especially since it can be done by accident pretty easily. To be fair, there are lots of gotchas in that territory. It might not be feasible to cover them all.

@Component(
  selector: 'some-cmp',
  template: '''
    <child-cmp></child-cmp>
    <child-cmp></child-cmp>
    <child-cmp></child-cmp>
  ''',
  directives: [ChildCmp],
)
class SomeCmp {
  @ViewChildren(ChildCmp)
  List children; // <------------- accidentally List<dynamic>

  void doStuff() {
    for (final child in children) {
      child.public; // sure
      child.privateToTemplate; // less good...
      child.recentlyRemovedField; // yikes!
    }
}

[matanlurey]

@bencalabrese:

I agree: these cases are much less common. Maybe this case would be served well enough by additions to the style guide describing what should be considered part of a components public API vs what is expected to be private to the component's template?

That makes sense to me. If you have any ideas for what should be a starting point, start a PR (or even just an issue, if you'd like).

We're starting to get into more complicated territory, but regarding > 1 component per dart library, could something along the lines of

Yeah, that's possible. But as you mention it falls apart for inline templates. I wouldn't want 3 ways to do something :-/

Part of me is inclined to say that if someone is casting to dynamic they're taking responsibility for whatever happens thereafter

That is my position as well.

[bencalabrese]

Thanks, @matanlurey.

Started an issue for discussion of what the guidelines should be: https://github.com/dart-lang/angular/issues/1509.

[matanlurey]

As per @srawlins, this has landed in 2.1.0-dev.2.0.

We will export and make the annotation available to end-users in the first release that supports that SDK.