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

Vend mixins for listeners block and hostAttributes block #4632

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@azakus
Copy link
Member

azakus commented May 25, 2017

Allow for easier migration to Polymer.Element for elements that use legacy hostAttributes and listeners blocks in Polymer({})

Fixes #4625

azakus added some commits May 24, 2017

Integrate with existing test infrastructure for listeners and hostAtt…
…ribures

HostAttributes: PropertyAccessorsMixin -> PropertyAccessors

@googlebot googlebot added the cla: yes label May 25, 2017

@azakus azakus requested review from notwaldorf , sorvell and cdata May 25, 2017

@notwaldorf
Copy link
Member

notwaldorf left a comment

OMG YES YES YES 👍

@notwaldorf
Copy link
Member

notwaldorf left a comment

love 2 document. some notes maybe?

* the "hostAttributes" configuration property used in Polymer 1.x.
*
* By returning a map from a static class property named "hostAttributes"
* , the custom element will ensure that the given attributes are set to given value if

This comment has been minimized.

@notwaldorf

notwaldorf May 30, 2017

Member

lol comma first eh

* }
* ```
*
* An instance of that element will have an attribute named "foo"

This comment has been minimized.

@notwaldorf

notwaldorf May 30, 2017

Member

I forgot to read this sentence because it was too spaced out, and too close to the type (so i assumed it was a compiler thing.

How about "For example, if an element implements hostAttributes like below, then an instance of that element will boot up with an attribute named "foo"`

* }
* ```
*
* An instance of that element will have a "foo" event handler that calls "fooHandler" on that element.

This comment has been minimized.

@notwaldorf

notwaldorf May 30, 2017

Member

same as before


<script>
class XBasicLite extends Polymer.mixinBehaviors([PropertyTypeBehavior], Polymer.HostAttributes(Polymer.Element)) {
static get hostAttributes() {

This comment has been minimized.

@sorvell

sorvell May 31, 2017

Member

Add a test for subclassing that shows inheriting/overriding.


<script>
class XListenersLite extends Polymer.mixinBehaviors([EventLoggerImpl], Polymer.Listeners(Polymer.Element)) {
static get listeners() {

This comment has been minimized.

@sorvell

sorvell May 31, 2017

Member

Add a test for subclassing that shows inheriting/overriding.

*/
class HostAttributes extends attributeBase {
ready() {
let hostAttributes = this.constructor.hostAttributes;

This comment has been minimized.

@sorvell

sorvell May 31, 2017

Member

This duplicates code in legacy-element-mixin. Ideally, it wouldn't and legacy-element-mixin would use this mixin. Alternatively, this could just be moved to another repo and then the implementation can diverge slightly.

*/
class Listeners extends listenerBase {
ready() {
let listeners = this.constructor.listeners;

This comment has been minimized.

@sorvell

sorvell May 31, 2017

Member

Subclassing is tricky with this implementation. Ideally you can easily inherit/override these.

@MajorBreakfast

This comment has been minimized.

Copy link
Contributor

MajorBreakfast commented Jun 20, 2017

I hope that this lands. The new way is definetly not DRY because you have to repeat yourself when you use bind() in the constructor, addEventListener() in connectedCallback and removeEventListener() in disconnectedCallback.

Edit: I don't think this is needed anymore. See my comment below.

@sz332

This comment has been minimized.

Copy link

sz332 commented Oct 2, 2017

Any information about the status of this PR?

@arthurevans

This comment has been minimized.

Copy link
Contributor

arthurevans commented Oct 2, 2017

@azakus Ping? Is this landing? If so, we could ref it from the docs.

@MajorBreakfast

This comment has been minimized.

Copy link
Contributor

MajorBreakfast commented Oct 3, 2017

I did some investigating and I don't think that adding the listeners object is sensible anymore:

You only need bind() when calling addEventListener() on something else than the custom element itself (e.g. window). The this inside the callback will be correct as the custom element is the current target of the event (and the current target is automatically used as this). Take a look at the code example below.
The listeners object only ever worked for listening for events on the custom element itself or elements with an id within the template.
The first can easily be accomplished by using this.addEventListener() in ready() in the fashion described above. Clean code. This also won't lead to a memory leak, because the element and callback only hold references to each other; there's no external reference. We would get an external reference if we called addEventListener() on window, but, again, the listeners object was never able to set up listeners on something external like window. Then, you need to start and stop listening inside the connectedCallback() and disconnectedCallback() and use bind() to capture the this.

ready() {
  super.ready();
  this.addEventListener('click', this._onClick);
}

_onClick(event) { /* ... */ }

The second can be achieved by using the on-myevent syntax. E.g. <div on-click="_onClick"></div>. This is also very clean.

In conclusion I can say that both use cases of the listeners object have a clean alternative. There's no need to reintroduce it.

I've already submitted a PR to improve this in the documentation Polymer/old-docs-site#2343 Currently it's a bit misleading.

@sz332

This comment has been minimized.

Copy link

sz332 commented Oct 3, 2017

@MajorBreakfast but why ready? Why not connectedCallback ?

@MajorBreakfast

This comment has been minimized.

Copy link
Contributor

MajorBreakfast commented Oct 3, 2017

connectedCallback(), unlike ready(), may be called more than once. It's called every time you attach your element. You'd end up with multiple listeners if you used that.

class MyElement extends HTMLElement {
  connectedCallback () { console.log('connect') } // Note: this.super() only needed for Polymer.Element
  disconnectedCallback () { console.log('disconnect') }
}
customElements.define('my-element', MyElement)
const el = document.createElement('my-element')
document.body.appendChild(el) // Output: 'connect'
document.body.appendChild(el) // Output: 'connect', 'disconnect', 'connect'
@sz332

This comment has been minimized.

Copy link

sz332 commented Oct 3, 2017

@MajorBreakfast thank you, great explanation. Probably this should be also added to the documentation. And just being curious: why not in constructor?

@MajorBreakfast

This comment has been minimized.

Copy link
Contributor

MajorBreakfast commented Oct 3, 2017

I think it works within the constructor just as well. Without Polymer this is probably the preferred place to add your listeners (because without Polymer there's no ready()). ready() runs a bit later (first time the element is added to the DOM). But, there's absolutely no difference if no events that you listen to are fired. Maybe setting them up in ready() has an advantage, but I don't see it. Works either way.

@sz332

This comment has been minimized.

Copy link

sz332 commented Oct 3, 2017

@MajorBreakfast if your add the listener in connectedCallback, but remove it in disconnectedCallback, is this okay?

@MajorBreakfast

This comment has been minimized.

Copy link
Contributor

MajorBreakfast commented Oct 3, 2017

You can do that. But this approach is only necessary (it's more verbose after all) if you listen on something like window, because there you'd run into a memory leak if you did it the way with ready(). Also, be sure to do it exactly as stated in the Polymer docs https://www.polymer-project.org/2.0/docs/devguide/events#imperative-listeners (Removal of the listener only works if you hold on to the function that bind() returns. That's why they store it in a property)

@sz332

This comment has been minimized.

Copy link

sz332 commented Oct 3, 2017

@MajorBreakfast would it be possible to extend the documentation with the information you shared in this PR? Because it was very enlightening, and now I understand how the event listener works, and probably others had also problem with this. Thanks.

@MajorBreakfast

This comment has been minimized.

Copy link
Contributor

MajorBreakfast commented Oct 3, 2017

@sz332 I've recently submitted this as a PR https://github.com/Polymer/docs/pull/2343/files It's not merged yet. Also, I've added the information to use ready() instead of connectedCallback() just now.

@azakus

This comment has been minimized.

Copy link
Member

azakus commented Dec 21, 2017

I'm going to drop this PR. Much of the functionality is easy to accomplish without it as @MajorBreakfast has noted.

@azakus azakus closed this Dec 21, 2017

@azakus azakus deleted the add-hostattributes-and-listeners-mixins branch Dec 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment