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

Expose "@visibleForTemplate" (for 2.1.0 SDK) #930

Closed
matanlurey opened this issue Feb 26, 2018 · 26 comments

Comments

@matanlurey
Copy link
Contributor

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

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

@jonahwilliams

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

@jonahwilliams:

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

Yes, but I don't control that :)

@jonahwilliams

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

@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

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented May 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2018

The first PRs have gone in. We are waiting for SDK support on dart-lang/sdk#33353 now.

@matanlurey matanlurey added this to the =v5.1.0 milestone Jul 7, 2018

@bencalabrese

This comment has been minimized.

Copy link

commented Jul 17, 2018

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?

dart-lang/sdk#33353 (comment) 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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

@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

This comment has been minimized.

Copy link

commented Jul 17, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

@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

This comment has been minimized.

Copy link

commented Jul 19, 2018

Thanks, @matanlurey.

Started an issue for discussion of what the guidelines should be: #1509.

@matanlurey

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

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.

@matanlurey matanlurey assigned matanlurey and unassigned srawlins Aug 9, 2018

@matanlurey matanlurey changed the title Add "@visibleForTemplate" (or similar) Expose "@visibleForTemplate" (for 2.1.0 SDK) Aug 9, 2018

leonsenft added a commit that referenced this issue Sep 11, 2018

@leonsenft leonsenft closed this in c91efa5 Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.