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

Implement property change notification events (notify) #81

Closed
FluorescentHallucinogen opened this Issue Jun 1, 2018 · 11 comments

Comments

Projects
None yet
6 participants
@FluorescentHallucinogen
Contributor

FluorescentHallucinogen commented Jun 1, 2018

What about something like Polymer's notify (automatic property change notification events) to make upward data flow easier?

Problem:

lit-element (lit-html) doesn't support 2-way binding. The correct way to pass data upwards is to use events.

E.g. for <paper-toggle-button> I can listen on-checked-changed, and then create a handler that sets the toggled property to the value I get from the event:

static get properties() {
  return {
    toggled: Boolean
  }
}

_render({toggled}) {
  return html`
    <paper-toggle-button
        checked="${toggled}"
        on-checked-changed="${this.onCheckedChanged.bind(this)}">
    </paper-toggle-button>

    ${toggled}
  `;
}

onCheckedChanged(e) {
  this.toggled = e.detail.value;
}

But what if I want to use <mwc-switch> (which is also based on lit-element) instead of <paper-toggle-button>?

The problem is that <mwc-switch> has no the checked-changed custom event unlike <paper-toggle-button>.

(The fact that <mwc-switch> has no the checked-changed custom event, shows that material-web-components violate Property Change Events rule of the The Gold Standard Checklist for Web Components.)

I've found https://github.com/DiiLord/lit-element and it has notify.

What about to port it to this element? It helps implement property change notification events as easy as adding notify to property in static get properties().

@FluorescentHallucinogen

This comment has been minimized.

Show comment
Hide comment
Contributor

FluorescentHallucinogen commented Jun 1, 2018

@TimvdLippe

This comment has been minimized.

Show comment
Hide comment
@TimvdLippe

TimvdLippe Jun 1, 2018

Contributor

(The fact that has no the checked-changed custom event, shows that material-web-components violate Property Change Events rule of the The Gold Standard Checklist for Web Components.)

I would actually say: mwc-switch should start firing the event, rather than making any update to LitElement.

Contributor

TimvdLippe commented Jun 1, 2018

(The fact that has no the checked-changed custom event, shows that material-web-components violate Property Change Events rule of the The Gold Standard Checklist for Web Components.)

I would actually say: mwc-switch should start firing the event, rather than making any update to LitElement.

@FluorescentHallucinogen

This comment has been minimized.

Show comment
Hide comment
@FluorescentHallucinogen

FluorescentHallucinogen Jun 1, 2018

Contributor

@TimvdLippe You are right, I mean that adding a few lines of code to lit-element to implement notify can save a bunch of code lines and time for implementation of custom property change notification events in thousands of other web components.

Contributor

FluorescentHallucinogen commented Jun 1, 2018

@TimvdLippe You are right, I mean that adding a few lines of code to lit-element to implement notify can save a bunch of code lines and time for implementation of custom property change notification events in thousands of other web components.

@TimvdLippe

This comment has been minimized.

Show comment
Hide comment
@TimvdLippe

TimvdLippe Jun 1, 2018

Contributor

@FluorescentHallucinogen I think the _didRender(props, changedProps, prevProps) already can take care of that? It should provide you a full dictionary of property name to current and the previous value. You can then call it something like this (havent' run the code, but it is a gist of what I am trying to convey):

_didRender(props, changedProps, prevProps) {
  if (changedProps['checked']) {
    this.dispatchEvent(new CustomEvent('checked-changed', {detail: {prev: prevProps['checked'], current: this['checked'] } } };
  }
}

Acknowledged this is a little bit of a boilerplate, but I think introducing the wrong abstraction in LitElement is worse off than letting users decide how they want to implement this logic.

Contributor

TimvdLippe commented Jun 1, 2018

@FluorescentHallucinogen I think the _didRender(props, changedProps, prevProps) already can take care of that? It should provide you a full dictionary of property name to current and the previous value. You can then call it something like this (havent' run the code, but it is a gist of what I am trying to convey):

_didRender(props, changedProps, prevProps) {
  if (changedProps['checked']) {
    this.dispatchEvent(new CustomEvent('checked-changed', {detail: {prev: prevProps['checked'], current: this['checked'] } } };
  }
}

Acknowledged this is a little bit of a boilerplate, but I think introducing the wrong abstraction in LitElement is worse off than letting users decide how they want to implement this logic.

@FluorescentHallucinogen

This comment has been minimized.

Show comment
Hide comment
@FluorescentHallucinogen

FluorescentHallucinogen Jun 1, 2018

Contributor

@TimvdLippe

but I think introducing the wrong abstraction in LitElement is worse off than letting users decide how they want to implement this logic.

Why notify is wrong abstraction? Are you sure that all web component developers know how to implement this right?

The first time you do something, you just do it. The second time you do something similar, you wince at the duplication, but you do the duplicate thing anyway. The third time you do something similar, you refactor.

— from Refactoring: Improving the Design of Existing Code book.

Contributor

FluorescentHallucinogen commented Jun 1, 2018

@TimvdLippe

but I think introducing the wrong abstraction in LitElement is worse off than letting users decide how they want to implement this logic.

Why notify is wrong abstraction? Are you sure that all web component developers know how to implement this right?

The first time you do something, you just do it. The second time you do something similar, you wince at the duplication, but you do the duplicate thing anyway. The third time you do something similar, you refactor.

— from Refactoring: Improving the Design of Existing Code book.

@FluorescentHallucinogen

This comment has been minimized.

Show comment
Hide comment
@FluorescentHallucinogen

FluorescentHallucinogen Jun 1, 2018

Contributor

Maybe I misunderstand the purpose of lit-element…

LitElement uses lit-html to render into the element's Shadow DOM and Polymer's PropertiesMixin to help manage element properties and attributes.

Contributor

FluorescentHallucinogen commented Jun 1, 2018

Maybe I misunderstand the purpose of lit-element…

LitElement uses lit-html to render into the element's Shadow DOM and Polymer's PropertiesMixin to help manage element properties and attributes.

@pitus

This comment has been minimized.

Show comment
Hide comment
@pitus

pitus Jun 6, 2018

I actually have the same problem with lit-element, that none of them seem to fire any events that can be listened to by ancestors. I even tried to dispatchEvent(...) of my own, but to no avail - no events seem to leave my element and I can't find any documentation or examples that would actually work.
I also agree that having a notify: true on your property to dispatch on-Property-changed event is a good idea as it simplifies a lot of repeating code where an element needs to dispatch custom event. What are LitElements for if not to provide a simple, light-weight framework for building our own elements?

pitus commented Jun 6, 2018

I actually have the same problem with lit-element, that none of them seem to fire any events that can be listened to by ancestors. I even tried to dispatchEvent(...) of my own, but to no avail - no events seem to leave my element and I can't find any documentation or examples that would actually work.
I also agree that having a notify: true on your property to dispatch on-Property-changed event is a good idea as it simplifies a lot of repeating code where an element needs to dispatch custom event. What are LitElements for if not to provide a simple, light-weight framework for building our own elements?

@pshihn

This comment has been minimized.

Show comment
Hide comment
@pshihn

pshihn Jun 6, 2018

@pitus that's because some (and custom) events do not cross the shadow dom boundary by default. You need to set composed to true.

const event = new CustomEvent('my-event', { bubbles: true, composed: true });
this.dispatchEvent(event);

pshihn commented Jun 6, 2018

@pitus that's because some (and custom) events do not cross the shadow dom boundary by default. You need to set composed to true.

const event = new CustomEvent('my-event', { bubbles: true, composed: true });
this.dispatchEvent(event);
@pitus

This comment has been minimized.

Show comment
Hide comment
@pitus

pitus Jun 7, 2018

@pshihn - yes, I got that suggestion from @eskan already and it works, but it's NOT being used by mwc components at the moment so I can't listen to on-changed on the mwc-textfield for example, although on-input does work with the latest change to textfield (as it's already componsed according to @eskan).
However, adding a notify and observer attributes on properties might be very useful and time-saving for custom elements so I don't see why it's a bad idea to have these added to the LitElement?

pitus commented Jun 7, 2018

@pshihn - yes, I got that suggestion from @eskan already and it works, but it's NOT being used by mwc components at the moment so I can't listen to on-changed on the mwc-textfield for example, although on-input does work with the latest change to textfield (as it's already componsed according to @eskan).
However, adding a notify and observer attributes on properties might be very useful and time-saving for custom elements so I don't see why it's a bad idea to have these added to the LitElement?

@sorvell

This comment has been minimized.

Show comment
Hide comment
@sorvell

sorvell Jun 8, 2018

Member

We generally agree with the Property Change Events rule, but Polymer's notify function did not follow this rule since it fired events on every change so it's not appropriate to include here.

Right now we're disinclined to add any sugar over the native dispatchEvent for this type of thing since it's typically under very specific circumstances that an element needs to fire an event due to an internal change. If there's a common pattern that emerges here, we may reconsider this.

Since specific issues noted here are actually with the mwc elements, we're closing this issue in favor of material-components/material-components-web-components#63.

Member

sorvell commented Jun 8, 2018

We generally agree with the Property Change Events rule, but Polymer's notify function did not follow this rule since it fired events on every change so it's not appropriate to include here.

Right now we're disinclined to add any sugar over the native dispatchEvent for this type of thing since it's typically under very specific circumstances that an element needs to fire an event due to an internal change. If there's a common pattern that emerges here, we may reconsider this.

Since specific issues noted here are actually with the mwc elements, we're closing this issue in favor of material-components/material-components-web-components#63.

@sorvell sorvell closed this Jun 8, 2018

@anargu

This comment has been minimized.

Show comment
Hide comment
@anargu

anargu Jun 15, 2018

Doesn't work this.dispatchEvent for me. What is the correct way to fire an event from my custom Element?

anargu commented Jun 15, 2018

Doesn't work this.dispatchEvent for me. What is the correct way to fire an event from my custom Element?

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