webcomponents / gold-standard

1.03k stars 39 forks source link

Recommendations for generating change events when an attribute has updated? #1

Closed JanMiksovsky closed 7 years ago

JanMiksovsky commented 9 years ago

From @RobDodson:

Before I forget, I'm not sure if the checklist includes well behaved components firing change events when an attribute has updated, but I think this is important for data binding systems like Angular 2.

I assume the idea here is that if you create an instance of , then change the foo attribute, you may be expecting the element to raise a “change” event.

I'm not sure whether there's any behavior here that's recognized as good by enough people that we can craft a checklist item for it, but I'm open to the idea.

robdodson commented 9 years ago

I heard today that Polymer 0.8 might do this for you, @sjmiles is that true(ish)?

I realize this issue extends beyond Polymer because you could be creating a vanilla Web Component, but wanted to point that out if it is true

chiefcll commented 7 years ago

I think this can be closed out thanks to @robdodson article: http://robdodson.me/interoperable-custom-elements/

I'm on board for accepting those guidelines.

JanMiksovsky commented 7 years ago

I'm not ready to accept Rob's suggestion that components only raise events for user interactions. @robdodson and I will discuss that, and determine how we want to update the checklist.

JanMiksovsky commented 7 years ago

To kick off the discussion, I wanted to capture some questions I have on this topic.

Here's the relevant part of Rob's article:

When I showed this pattern off recently someone asked if it would be better to dispatch the events inside of the property setters, for example:

set checked(val) {  
  this._checked = val;
  /* do any other updates we need */
  this.dispatchEvent(new CustomEvent(‘check-changed’, {
      detail: checked: val
  }));
}

The danger with doing this is you may end up creating an infinite loop if there’s an external listener for the event which in turn updates the property. Looking again to native elements, it seems that their event dispatching only takes place when some external force is acting upon the element, e.g. someone clicked, someone typed, something loaded, etc. Hence, we don’t dispatch check-changed when the public property is set, but instead only when someone clicks on the checkbox.

First, I want to observe that Rob's writing about properties, while this issue is about attributes. So the first question is: do we want to treat those differently? That is, do we want component authors to raise events when an attribute changes, or when a property changes, or both? Those options are related, because most authors will have an attributedChangedCallback set the corresponding property.

A second question is: Does the standard HTML behavior make sense globally for an essentially infinite number of custom elements? A checkbox doesn't raise its change event when its checked property changes. The premise of the Gold Standard is to emulate the standard HTML elements, so a priori, we assume we want custom elements to behave the same way. I think that's a good starting point. But if it makes life difficult, I'm also open to reconsidering that stance. The world only has ~100 standard HTML elements, only a tiny number of which make up the bulk of interactive behavior. At that scale, documentation of special cases or behaviors is workable. But there will be zillions of custom elements, and it’s possible that what makes sense for that scale will be different.

A third, related question is: Is the standard HTML behavior really obvious? I actually hadn’t known (or had forgotten) that a checkbox doesn’t raise a change event when its checked property changes. That behavior was surprising to me. It’s probably surprising to other devs. And, from Rob’s wording, “It seems that their event dispatching only takes place…”, I infer that Rob didn’t know with absolute certainty how a really basic feature of a really basic component worked, and had to resort to poking at a checkbox to see what it did. Rob’s a super-smart guy, and let’s conservatively put him in the 99th percentile of knowledgable web developers. I’ll put myself in the 90th percentile. If we’re surprised or not sure when events fire in these cases, then I have to assume many other people will be even more confused. It seems much simpler to be able to say, “Whenever property foo changes, it raises a foo-changed event."

Fourth question: How would data binding systems (Polymer, Ember, etc.) work without change events for properties? From Polymer’s docs on change notification events, it seems that Polymer relies on change events being fired when there is a "change to a notifying property.” Without that, how can Polymer know that an underlying property value has changed and should be propagated along any existing bindings? I’m not familiar with Ember and similar frameworks, but assume they need such events too.

Fifth question: I’m not completely certain of this, but given the above issue, I’m concerned that people composing complex UI components from multiple, smaller components will have to carefully keep track of which things are happening in direct response to user interactions and which are happening because of changes imposed by the outer application. This is similar to the data binding question above; here the difference is that a component author might not be using a framework with data binding, but rather gluing the smaller constituent components together by hand. I’m worried such glue code will become more complex if the component author has to track the source of a property change.

Sixth question: How can component behavior be properly factored if there is a direct relationship between user interactions and property effects? Case in point: the Elix RFC for a single-selection mixin proposes a mixin that manages abstract single-selection semantics for components like lists. It’s analogous to Polymer’s iron-selectable-behavior, but by design the mixin is completely abstract, and has no connection to the DOM. We think that’s a good thing. The mixin presumes that changing a property like an object’s selectedItem should raise a selected-item-changed event. That is very easy to arrange for in the setter for selectedItem, and results in a simple API. But if the component should only raise change events in response to user interaction, the mixin needs some more complex API in which the base component needs to let the mixin know whether a change to selectedItem is happening as a result of user interaction (and should raise an event) or happening for some other reason. That seems quite complex.

Seventh question: How should components handle situations in which they update multiple properties in response to a single user event? Again using that single-selection mixin example, that proposal supports modifications of a component’s selection in two ways: 1) by item index or, 2) by an object reference to the item. These two properties are linked: if you change the selectedIndex property, the selectedItem property updates too, and vice versa. This compounds the issue raised in the above point.

Eighth question: Should the prevention of the infinite loop be the dominant concern here? Rob points out, "The danger with doing this is you may end up creating an infinite loop if there’s an external listener for the event which in turn updates the property.” The suggestion to not raise change events for property updates attempts to avoid the possibility of such an infinite loop. But that places a significant burden on the component author. Another solution is to push the responsibility on the component user: i.e., a change event handler shouldn’t update the indicated property on the original component.

robdodson commented 7 years ago

I spoke with some Blink engineers about this. Let me try to answer the above points in order.

So the first question is: do we want to treat those differently? That is, do we want component authors to raise events when an attribute changes, or when a property changes, or both? Those options are related, because most authors will have an attributedChangedCallback set the corresponding property.

As a general rule of thumb, I don't think we want every property/attribute to dispatch an event. To quote one of the Blink engineers: "In the platform today most 'property changed' events don't fire if you manually set the property because it's assumed that you were the one setting it." It would also be quite spammy and possibly expensive if every property change dispatched an event.

Having said that, there may be exceptions because the platform is not consistent here. Apparently setting the src property of an img dispatches a load event.

Does the standard HTML behavior make sense globally for an essentially infinite number of custom elements?

I think it's a good model to follow and it's what frameworks and libraries are already written to understand.

A third, related question is: Is the standard HTML behavior really obvious?

I guess for me it's non-obvious only because prior to Custom Elements I didn't spend much time building components to exactly model what the DOM does today. But I think it's a teachable pattern. Again, saying every property should dispatch an event will probably get very expensive/spammy.

Fourth question: How would data binding systems (Polymer, Ember, etc.) work without change events for properties?

The same way it listens to native checkbox today. Polymer's notify stuff is primarily for two-way data binding but two-way bindings are an opinionated, framework-specific pattern. It's not something the platform natively supports and it's not even something that all frameworks universally support or favor.

Fifth question: I’m not completely certain of this, but given the above issue, I’m concerned that people composing complex UI components from multiple, smaller components will have to carefully keep track of which things are happening in direct response to user interactions and which are happening because of changes imposed by the outer application.

This is how native HTML works today and I don't think we have major issues there. Each element I'm composing is essentially a black box to me, so I can ignore why it's dispatching its change event and just listen for it.

Sixth question: How can component behavior be properly factored if there is a direct relationship between user interactions and property effects?

I think this becomes less of an issue if we remove the mixin from the equation. Personally I question the value of behavior mixins. I realize not having mixins leads to code duplication in elements but I've seen first hand how elaborate and confusing mixins have made some Polymer Elements. I'm not a big fan but that's just my personal opinion :)

Does the mixin have to use an event dispatch? Could it use a callback instead? Maybe override the setter and whenever it gets called, at the very end do callback(this.selectedItem).

Seventh question: How should components handle situations in which they update multiple properties in response to a single user event?

Does removing the mixin make this easier to reason about?

Eighth question: Should the prevention of the infinite loop be the dominant concern here?

It seems fair to want the frameworks out there watch out for this. But I think another reason to avoid having every property fire an event would be to avoid spamminess.

I'm curious what @treshugart thinks about all this :D

JanMiksovsky commented 7 years ago

@robdodson Thanks for the thoughtful response! It's really helpful.

Since I don’t want opinions about mixins to color this discussion, let’s set that topic aside. At some point, I’d like to have the opportunity to show you why we think they’re actually a suitable pattern for complex components, and how we can avoid some of the issues Polymer ran into with behaviors — but that can wait.

Other points…

To quote one of the Blink engineers: "In the platform today most 'property changed' events don't fire if you manually set the property because it's assumed that you were the one setting it." It would also be quite spammy and possibly expensive if every property change dispatched an event.

Performance is a compelling argument. I’m willing to see if we can rewrite our components to be able to only generate change events for UI interactions. I’m concerned we’ll run into issues, but there’s no point in my arguing about that until I’ve tried and can speak directly to experience.

Does the standard HTML behavior make sense globally for an essentially infinite number of custom elements?

I think it's a good model to follow and it's what frameworks and libraries are already written to understand.

I remain concerned that what’s worked for a relatively tiny set of extremely well-known, simple elements may not scale to a much larger number of components which are possibly much more complex. In particular, I’m worried that it will often be desirable for components to expose two or more properties with implicit relationships. As just one example, let’s look at google-map:

The point is that the implicit relationships make it hard for the dev directing the action to know what can effect what. Suppose I’m gluing a google-map to both components that can control the map, and to components that reflect map properties like lat/long. Without property change events, I don’t know whether setting fitToMarkers will affect zoom. I have to do something like: if the map-controlling components want to change fitToMarkers, I remember to check to see whether zoom changed, and then propagate the change to update the components reflecting map properties.

With property change events, I would have just told the map property-displaying components to listen for change events in the properties they render. When my map-controlling components poke at the map, properties update, change events are raised, and new properties are rendered.

Here the sheer number of components matters. The small number of standard elements changes at a glacial pace, and there’s ample time to learn how those elements work. If, however, we're dropping new components into our projects on a daily basis, it may be very hard to know which properties will have implicit affects on other properties.

A third, related question is: Is the standard HTML behavior really obvious?

I guess for me it's non-obvious only because prior to Custom Elements I didn't spend much time building components to exactly model what the DOM does today. But I think it's a teachable pattern. Again, saying every property should dispatch an event will probably get very expensive/spammy.

I agree that this could be taught. I’d be happier with a simpler model (change a property by any means => raise event), but given the performance concerns you raise, I don’t see much of a choice.

Fourth question: How would data binding systems (Polymer, Ember, etc.) work without change events for properties?

The same way it listens to native checkbox today. Polymer's notify stuff is primarily for two-way data binding but two-way bindings are an opinionated, framework-specific pattern. It's not something the platform natively supports and it's not even something that all frameworks universally support or favor.

I just wanted to point out that the question isn’t necessarily about two-day data binding. In the map example above, the bindings (managed through a data-binding layer, or by hand) could be one way.

robdodson commented 7 years ago

ok so to summarize, if changing propA of an element causes it to internally change the value of propB, how does the outside world know that propB just changed.

The options are:

Does that seem about right? I'd like to get other folks opinions on this but want to try to distill the explanation down to something bite sized 😸

JanMiksovsky commented 7 years ago

Yes, I think you've captured that issue. Thanks for taking the time to boil this down.

robdodson commented 7 years ago

@esprehn do you have an opinion on https://github.com/webcomponents/gold-standard/issues/1#issuecomment-267209490

robdodson commented 7 years ago

I'm going to quote @kevinpschaaf from the Polymer team here just to add some opinions.

If we weren't bound by the [Polymer] 1.x behavior, I would likely push to codify the following as a best practice: never fire property-changed events as a result of an outside actor changing the property (aka downward data flow); only fire them as a result of an internal change not initiated from outside actors (e.g. UI interaction, async tasks, network/service results, etc.). Reasoning is similar to topics discussed in [this] thread: the potential for i-loop is high (we only avoid this in 1.0 through strict equality dirty-checking, which is restrictive), it can lead to change processing-reentry which is difficult to reason about, notifying a property set from the host back to the host is superfluous and perf-negative, and this is generally the behavior of platform events

JanMiksovsky commented 7 years ago

@robdodson and @kevinpschaaf: Thanks for the insights. We'll do our best to make this work, and raise any issues we find.

For the Gold Standard, I really want to offer prescriptive advice when possible about how to achieve a particular result. If we were to codify this and incorporate Kevin's advice, I'd feel better if there were a pattern devs could follow to implement it.

In particular, I want to make sure that the pattern can cover the cases like our complex components, where a handler for a user interaction event may invoke many layers of code that ultimately need to set a property. In such cases, we're concerned that it will difficult for a component author to properly raise property change events.

One simple pattern we're trying out is:

  1. A component sets an internal flag at the beginning of any UI event handler or other internal events the host can't know about. At the end of the handler, the flag is cleared.

    const handlingUserInteractionSymbol = Symbol();
    
    this.addEventListener('click', event => {
      this[handlingUserInteractionSymbol] = true;
      this.foo = ... // Set a property, or call routines that do.
      this[handlingUserInteractionSymbol] = false;
    });
  2. Any property setter can check this flag to detect whether the work it's doing is in response to a UI/internal event, and should therefore raise a property change event.
    get foo() {
      ...
    }
    set foo(value) {
      ...
      if (this[handlingUserInteractionSymbol]) {
        this.dispatchEvent(new CustomEvent('foo-changed'));
      }
    }

    See http://jsbin.com/nopafu/edit?html,console,output.

This pattern:

Does this pattern meet the criteria? Would you feel comfortable seeing this pattern in the Gold Standard treatment of this recommendation?

domenic commented 7 years ago

Maybe this is a good place to mention that users might expect canceling events to undo the change, like it does for input/change events in the DOM.

JanMiksovsky commented 7 years ago

@robdodson I've taken a shot at adding a new guideline to the Gold Standard called Property Change Events. Per your recommendation, this indicates that property change events should only fire in response to internal component activity, and not when properties are set externally by the host. Can you please review?

@domenic I hadn't known that was possible. The MDN docs for change and input indicate the events are not cancelable. I also tried a spike and, per spec, don't see attempts to cancel the events succeeding. Can you update that example so I can see canceling of an input or change in practice?

domenic commented 7 years ago

@JanMiksovsky thanks for checking me; I was confused. It's the click event that's cancelable, and canceling it will undo the change on checkboxes and radio buttons. On reflection I think that's esoteric enough that it should be thought of more as a legacy mistake than as something expected. It's even named as such ("legacy-canceled-activation behavior").

JanMiksovsky commented 7 years ago

@domenic Ah, okay. Thanks for the follow-up.

robdodson commented 7 years ago

The recommendation in the wiki looks good to me.

JanMiksovsky commented 7 years ago

Thanks! Since this has now been incorporated into the checklist, I'll close this issue out.

robdodson commented 7 years ago

👏 great work @JanMiksovsky !

treshugart commented 7 years ago

Sorry I never got a chance to weigh in, @robdodson, but thanks for cc'ing me. Most everything you've put down I'd echo, though, due to the holidays I haven't had a chance to give it the attention I probably should have. Great work @JanMiksovsky :)

jouni commented 7 years ago

@robdodson, what’s Polymer’s approach to this? Will the behaviour of notify: true be updated according to this recommendation at some point?

robdodson commented 7 years ago

@jouni that's a good question. @kevinpschaaf do you know if Polymer 2 will change this behavior?

kevinpschaaf commented 7 years ago

@jouni @robdodson Polymer 2.0 is intended to be mostly backward compatible w.r.t. the features in 1.x, so the notify:true behavior won't change. Regardless, its hard to envision how the notify: true behavior could be adapted to meet the recommendation since there's not enough information to know when the accessor is running whether the change was from the outside (e.g. from the host) or inside (e.g. as a result of internal event handling).

The recommendation really argues for not putting the event dispatch as a property-change side-effect (what notify: true does) and instead puts the onus on the author to properly dispatchEvent a change event when the element changes properties as a result of event handling. I think we would probably start there (manual), and then consider what sugar would help promote the pattern.

JanMiksovsky commented 7 years ago

@kevinpschaaf FWIW, I was worried about adapting our components to support this recommendation. However, the simple raiseChangeEvents flag pattern described on the Property Change Events to discriminate between outside and inside property changes. This pattern appears to be working for us so far.

Polymer could consider whether it could define a similar flag that could be checked by its auto-generated setters for notify: true properties. There's a backward-compat problem: the raiseChangeEvents flag only works if its default value is false, and that's the opposite of the way notify: true works. But maybe there's a way to make that work.

kevinpschaaf commented 7 years ago

@JanMiksovsky That's fair, but in either case given that you must do something to say whether an event should be fired, given the choice between twiddling some sugar or just doing the platform-idiomatic thing (call dispatchEvent where you want the event dispatched), at this point I'd probably just argue for less sugar. But yeah, that's an approach.

JanMiksovsky commented 7 years ago

My experience has been that situating property change event dispatchers inside, e.g., UI event handlers is challenging. If you're designing an accessible component, for example, you'll end up with both mouse/touch and keyboard events. In complex components, it can also be hard to anticipate whether changing one property in a click handler will have second-order effects on other properties, so the click handler might have trouble figuring out just which property change events to raise. Finally, a UI event handler shouldn't raise a property change event if the property didn't actually change, and tracking that is a chore that a property setter may already be handling.

This experience has led to the raiseChangeEvents pattern, which divides responsibility for raising property changes events between the UI event handler (which just needs to signal that internally-triggered work is about to occur) and the property setter (which can look to see if a property has actually changed, and which can raise property change events in all circumstances).

YMMV, but it's working for us.