Skip to content

Allow mixin classes to have members added via .prototype calls.#799

Merged
aomarks merged 3 commits intomasterfrom
mixin-prototype
Dec 3, 2018
Merged

Allow mixin classes to have members added via .prototype calls.#799
aomarks merged 3 commits intomasterfrom
mixin-prototype

Conversation

@aomarks
Copy link
Copy Markdown
Member

@aomarks aomarks commented Dec 1, 2018

For example:

  /** @type {string} */
  MyMixin.prototype.foo;

We already support this behavior for normal classes, but not for mixin classes.

We need this because we're going to change some class mixin properties such as PolymerElement._template to not be initialized in the constructor, but we still need a way to tell analyzer that they exist.

cc @sorvell @kevinpschaaf

@aomarks aomarks requested a review from rictic December 1, 2018 01:44
Demonstrates problem where analyzer does not detect mixin class
properties that are added to the prototype.
For example:

  /** @type {string} */
  MyMixin.prototype.foo;

We already support this behavior for normal classes, but not for mixin
classes.
@rictic
Copy link
Copy Markdown
Contributor

rictic commented Dec 2, 2018

We can also do this via:

/** @type {string} */
this.foo;

in the constructor, which I'd argue is more legible. I guess if that's the only thing in the constructor it might be better for the VM to leave it off though?

@aomarks
Copy link
Copy Markdown
Member Author

aomarks commented Dec 2, 2018

We can also do this via:

/** @type {string} */
this.foo;

in the constructor, which I'd argue is more legible. I guess if that's the only thing in the constructor it might be better for the VM to leave it off though?

Yeah @kevinpschaaf and @sorvell specifically wanted to avoid having the property accesses at all in the constructor, for uncompiled users.

@aomarks aomarks merged commit ce5ca6c into master Dec 3, 2018
@aomarks aomarks deleted the mixin-prototype branch December 3, 2018 21:47
aomarks added a commit to Polymer/polymer that referenced this pull request Dec 6, 2018
Only significant change is from Polymer/tools#799
aomarks added a commit to Polymer/polymer that referenced this pull request Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants