Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Should adapter_, root_ and foundation_ be @protected instead of @private? #767

Closed
nckh opened this issue Jun 1, 2017 · 5 comments
Closed

Comments

@nckh
Copy link
Contributor

nckh commented Jun 1, 2017

According to Closure's annotating documentation, protected properties are available to instance methods of any subclass. With @private, compilation seems to trigger a warning ("Access to private property adapter_ of XXX not allowed here").

@traviskaufman
Copy link
Contributor

They absolutely should. In fact, I believe I had it properly annotated within my experimental/closure-research branch: https://github.com/material-components/material-components-web/tree/experimental/closure-research/packages/mdc-base

Would you be willing to submit a PR with these changes? I also believe that both MDCComponent and MDCFoundation should be marked as @abstract.

@nckh
Copy link
Contributor Author

nckh commented Jun 1, 2017

Sure, I'll take it.
If I remember well, the compiler complains about @abstract because an instance of MDCComponent is created in MDCComponent.attachTo.

@lynnmercier
Copy link
Contributor

the compiler complains about @abstract because an instance of MDCComponent is created in MDCComponent.attachTo.

Thats exactly the problem I saw when I tried marking them @abstract.

@traviskaufman
Copy link
Contributor

In terms of attachTo, I'm actually fine just having it return a plain object by default. attachTo on MDCComponent really doesn't serve any purpose other than documenting what a static API for components should look like.

@nckh
Copy link
Contributor Author

nckh commented Jun 5, 2017

Returning a plain object will throw a warning since the expected return type is {!MDCComponent}. No warning if the function body is left empty, and more importantly, the annotations on static methods don't seem to be inherited by subclass implementations. For example the broken code below does not throw any warning.

/**
 * @abstract
 */
class MDCComponent {
  /**
   * @param {!Element} root
   * @return {!MDCComponent}
   */
  static attachTo(root) {}
}

/**
 * @extends {MDCComponent}
 */
class MyComp extends MDCComponent {
  /** @override */
  static attachTo(root) {
    return 1234;
  }
}

console.log(MyComp.attachTo(null));
  • MyComp.attachTo returns a number even though it's supposed to return an instance of MDCComponent
  • null is passed to MyComp.attachTo even though it only expects non-null Elements.

Changing attachTo to instance methods and suddenly the compiler complains about both issues. Not sure if it's a bug, a specificity of static methods, or me missing something.

Removing @override and carrying over the annotations from the parent class does fire warnings correctly.

/**
 * @extends {MDCComponent}
 */
class MyComp extends MDCComponent {

  /**
   * @param {!Element} root
   * @return {!MDCComponent}
   */
  static attachTo(root) {
    return 1234;
  }

}

But in this case, is defining MDCComponent.attachTo still useful?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants