-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Forwarding Element Modifiers with "Splattributes" #435
Conversation
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. |
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 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 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 ( |
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. |
@simonihmig: Glimmer components does not have 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 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 <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). |
@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.
Does that apply specifically also to element modifiers, especially the order in which their lifecycle hooks are called?
Is
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! 🙂 |
@simonihmig @knownasilya @CvX @les2 Thanks for the discussion! I tried to incorporate your feedback into the RFC text, let me know what you think! |
cc @cibernox (in relation to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chancancode excellent summary of my concerns! 👏
Co-Authored-By: chancancode <godfreykfc@gmail.com>
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. |
This is a bit of an experiment, to do a small amendment to merged RFCs with a quick round of FCP.
Rendered