Forwarding Element Modifiers with "Splattributes"

[chancancode]

This is a bit of an experiment, to do a small amendment to merged RFCs with a quick round of FCP.

Rendered

[wycats]

At the core team meeting, we agreed that this change could be considered a technical correction because of the interaction between two concurrent RFCs, and that it is immediately ready for final comment period.

[simonihmig]

I think it makes sense, though I want to mention a possible pitfall here...

The basic "problem" here is that we are loosening the black box principle of a component (we already did with splattributes I guess, see example below). By that I mean previously you had no way to interfere with the inner structure of a component (its DOM, event handling etc.). At least not in an "ember way", without messing with its DOM directly. It was a black box, and the component could think of itself as the solely owner of everything beneath it.

Now with forwarding element modifiers, we are exposing the internals in a way, which could potentially lead to problems. Example:

<button {{on click=(action this.onClick)}} ...attributes>Click me</button>

Now let's assume the user passes another element modifier (or even the same) on the component's invocation, which forever (bad) reason uses event.stopImmediatePropagation() in its attached event handler. This will lead to a conflict, with one of the event handlers not being called, depending on the order the element modifiers are applied (or rather their lifecycle hooks being called).

Btw, for cases like that I think it would make sense to specify the order/precedence in which element modifiers are attached and their lifecycle hooks called!?

FWIW, this "black box" principle was probably already violated in a way with regular splattributes. Imagine a form control component that renders something like this:

<input type={{@type}} id={{this.myUniqueId}} ...attributes>
<label for={{this.myUniqueId}}>
  {{yield}}
</label>

Passing an id to the component would apply it to the input (as expected), but (unexpectedly) break the accessibility by disconnecting the label from it.

Allowing to forward element modifiers just widens the possibilities for conflicts like that. Having said that, I still think that these would be rather edge cases, and mostly results of bad implementations (event.stopImmediatePropagation()). But at least one should be aware of this, and maybe add this to the guides, that element modifiers should expect that they are not the only ones, that they don't own "their" element, and be designed to play nicely with others (e.g. others might further override their attribute changes)

[rwjblue]

Thank you @simonihmig! I agree that it’s important to consider, however I still think this is the right path forward.

The changes proposed here do not introduce new challenges (arguably the fact that attributes can be clobbered is a much larger issue and was an intentional choice in the angle bracket invocation RFC) and I believe they continue to improve the composability that we intended to unlock with angle bracket invocation.

[chancancode]

@simonihmig: Glimmer components does not have ...attributes applied to any specific element by default (unlike Ember.Component, which always applies it to the Ember-controlled root element unless tagName=""). This means that they are "black boxed" (in the sense you described) by default, and up to the component author to decide which, if any, elements to apply them on.

I personally think the benefits almost always outweighs the downsides and it's somewhat of an anti-pattern to not do it, but it's still opt-in, so the component author can keep the component black boxed if it's important for their use case.

However, as you pointed out, since we are not using shadow DOM, there is really nothing preventing the users from messing with the DOM directly anyway, with or without splattributes. If we allowed adding attributes only, you can pretty much accomplish anything you would have wanted to accomplish with modifiers by adding a unique data- attribute and use query selectors to find them.

Ultimately, I think it's definitely a bit of a sharp tool, but I think the community will quickly settle on certain best practices (such as don't prevent default). Ultimately, if you do anything the component author doesn't want you to do, you are probably just going to break the component and hence your own app, so I think the incentives are aligned here.

As for ordering, they are specified in RFC #311 – they are applied in the same order that the attributes are given. This also means you could avoid your id clobbering problem by putting overridable attributes first, followed by ...attributes, followed by non-overridable attributes:

<input type={{@type}} ...attributes id={{this.myUniqueId}}>
<label for={{this.myUniqueId}}>
  {{yield}}
</label>

Of course, you can still override it with a modifier since they run last, but then again, modifiers are a powerful sharp tool that comes with great responsibilities. I agree with all the things you said about teaching modifiers. I'm not sure if I should include it in this RFC though, since they are really just about modifiers themselves and not really related to splattributes (you can have multiple of them on a regular element, so the playing nice part is basically always important).

[simonihmig]

@chancancode thanks for the thorough feedback here!

As I said in my first sentence, I do support this RFC, and just wanted to point out possible problems, which we have to teach properly.

As for ordering, they are specified in RFC #311 – they are applied in the same order that the attributes are given

Does that apply specifically also to element modifiers, especially the order in which their lifecycle hooks are called?

<MyComponent {{mod1 "foo"}} {{mod2 "bar"}}/>

Is mod1#didInsertElement() guaranteed to be called before mod2#didInsertElement()? That was my main question.

I'm not sure if I should include it in this RFC though, since they are really just about modifiers themselves and not really related to splattributes (you can have multiple of them on a regular element, so the playing nice part is basically always important).

Yes, agree. However I do think that modifiers that are not playing nice can lead to more unexpected and hard to debug problems with splattributes. Think of using them with a component that comes from an addon. In your own app's template, you would see two modifiers being applied to a static element, so it's easier to reason about what could have caused some failing behavior. But as an addon's component is much more of a black box, when you apply one to it which is not playing nice (e.g. preventing event bubbling), you have a harder time figuring out what's happening as it's not so easy to see how the modifier is interacting with the components implementation, and will maybe wrongly blame the addon that it's not doing what is expected.

So my tl;dr is just that teaching this properly is important, even more so with splattributes! 🙂

[chancancode]

@simonihmig @knownasilya @CvX @les2 Thanks for the discussion! I tried to incorporate your feedback into the RFC text, let me know what you think!

[chancancode]

cc @cibernox (in relation to the element helper)

[simonihmig]

@chancancode excellent summary of my concerns! 👏

[wycats]

We discussed this in the core team meeting, and think this is ready to merge. As it currently stands, this is a sharp tool (as described in the RFC's "How We Should Teach It" section), and it makes sense to add higher level functionality as the usage patterns become more apparent.