emberjs / rfcs

RFCs for changes to Ember
https://rfcs.emberjs.com/
791 stars 408 forks source link

...attributes -- Component API and Discoverability #586

Closed NullVoxPopuli closed 2 years ago

NullVoxPopuli commented 4 years ago

A couple co-workers brought up some interesting points this morning.

Given that ...attributes can be placed anywhere in a component

You

Questions for the community:

The main thing we're after here is clarity without having to know implicit community convention -- because implicit community convention can't be programmatically enforced atm.

Related: https://github.com/ember-learn/guides-source/issues/1286 (the specific component in that link, Input, isn't important to this issue, but the problem is there)

chriskrycho commented 4 years ago

My take here is: this should become standard part of API documentation for add-ons, internal and external. But, and this is key: what happens to the ...attributes should not always be documented. In many cases, it's effectively internal implementation details, same as "what exactly do you do with @bar once I hand it to you?"

  • should we add a lint rule to suggest that ...attributes should only be used in the top-most element?
  • should we have some additional tooling ((u)ELS?) to tell us if ...attributes is used, if any @args overwrite / merge any of the attributes.

Almost certainly not, at least as a default. These are, again, internal details. You might have very good reasons for choosing always to stomp certain things the user might pass in. E.g. in maintaining accessibility, if the user passes in values that would break your internal invariants about how certain elements are related, you want to be able to guarantee that your invariants win.

MichalBryxi commented 4 years ago

Disclaimer: I'm the mentioned coworker from the first comment.

I feel that the hiding of "implementation details" might be problematic. Let's say I found an addon that has component <Foo /> that has arguments @bar and @baz. And <Foo /> contains more than one HTML tag. Now I want to add some HTML attribute to achieve X, because X is not supported by the arguments provided. Maybe:

Whichever attribute from my list I chose to implement, it will depend on which HTML element it is being applied to (not all of them are applicable after all). And how are they applied (why is my value not here? oh it's being overriden :sad-panda:).

And at that point I as a consumer have to go and look at the source code, because it is not obvious from the syntax and it is (usually) not in the docs.

I'd say that if we force the consumer of the addon/component to look at the source code of the addon because the syntax OR the docs are not being explicit enough, we're doing the documentation wrong.

ro0gr commented 4 years ago

These are, again, internal details.

I believe it should not be like that. Allowing an "addon" author to place ...attributes wherever they want feels like a footgun to me. In its current form It may easily lead to breakages of HTML validity or introduce styling regressions. There are few examples:


Let's assume we render smth like:

<SomeonesSuperComponent id="myId" />

I'm pretty sure it should result to a single element being rendered with an ID passed to it.

However, If the author of "SomeonesSuperComponent"'s decides to apply ...attributes multiple times in their template, it would break HTML, cause we can't have the same ID on the same page.

It would break HTML, cause we can't have the same ID on the same page.


Similar happens when you just want to give some outer styling to your component:

<style>
  .my-component {
    margin: 10px;
  }
</style>

<div>
  <SomeonesSuperComponent class="my-component" />
</div>

with that ^, I, as a developer, expect to have my SomeonesSuperComponent's outer element to have 10px of margin within a parent div. However, a SomeonesSuperComponent's author may decide to apply ...attributes somewhere deeper than top-most element, which would break my expectations and design :(


The last but not least. From time to time, I encounter that some of my tagless component templates are missing ...attributes. As a result, when I render <SomeonesSuperComponent it just doesn't accept any attributes, which is highly unexpected to me.


TLDR; I believe there should not be a way to apply ...attributes somewhere else then a top-most element of a tagless component. Only once per template.

pzuraq commented 4 years ago

@ro0gr FWIW, we spent a long time discussing that as a possibility in the community during the lead up to adding angle bracket syntax and the ...attributes keyword. I think reading back in some of the older RFCs might be a good way to get some context on those older discussions, and the reasons why it was decided to not place these restrictions on the usage of ...attributes.

That said, if it turns out that there are antipatterns like you're describing, I think that's a perfect place for linting rules 😄 Still allow the flexibility, but try to have a general rule of thumb for best practices.

ro0gr commented 4 years ago

@pzuraq yes, i observed that discusisions, and I think, I just haven't realized all the implications good enough. I'm not complaining, just sharing my findings.

I agree, we can try to reduce misuse(in my opinion) of attributes via linting, but I'm unsure if it can distinct between tagless VS tagName components...

I think, Ideally, i'd be happy, if I could skip writting it at all, and have default behavior applied implicitly in case of a single root(95% of my cases?), until explicitly specified "...attributes". Not sure if it's feasible though, and which layer's resonsibility should it be.

pzuraq commented 4 years ago

That was one of the options that was discussed. I believe the reason we didn't choose it was because, as time has gone on, we've preferred more explicit APIs. Implicit APIs like that often come back to bite you.

For instance, imagine if a junior Ember dev saw a template for a component like this:

<section>
  <!-- ... -->
</section>

And was tasked with adding a new header above the section. They go ahead and add it, without realizing that there is implicit behavior being caused by the current layout, and then things suddenly break 🙃

With explicit syntax and rules, you avoid breakage (because it's explicit, and allowed), and you give a hint to developers that something is special about this particular element.

ro0gr commented 4 years ago

I feel, if someone pass attributes to the component with multiple root elements under irs template, rendering should fail. Otherwise, the user of such component can not rely on patterns used in regular HTML/CSS

ro0gr commented 4 years ago

It can also prevent a junior to make a wrong thing

pzuraq commented 4 years ago

@ro0gr that would be one way to solve these problems, and we did discuss those solutions at the time. I think making those changes at this point would be very significant breaking changes, so it's unlikely that it will happen.

Happy to talk through why the decisions were made, and what the perceived pros and cons were, but personally I think it'd be best to keep the discussion here focused on figuring out best practices with the existing behavior.

ro0gr commented 4 years ago

keep the discussion here focused on figuring out best practices with the existing behavior.

@pzuraq Unfortunatelly, I don't think there can be good practices to apply ...attributes on something different than outer-most element.

If there is no single outer element, I'd not recommend to use ...attributes at all. Otherwise, this component just becomes unreliable to style. The same is valid for cases when ...attributes applied somewhere deep in the component markup. Also, at some point you may realize, you are missing an outer element attributes, and duplicate ...attributes, and that's where things start going totally unmanaged. I'd be happy to see an example when ...attributes are really useful outside of outer-most element, and don't cause more problems than benefits.

Though, I believe cases like that are pretty rare in real apps. They are so special that it seems more reasonable to just use arguments explicitly.

That being said, I believe, we should discourage community from patterns like that, and template lint can be a great tool for that! And, maybe, at some mid/long-term point, we can even disallow such a freedom for ...attributes completely.

Gaurav0 commented 4 years ago

One exception tends to be input / textarea elements with surrounding and/or sibling labels and error messages. In that case, ...attributes makes more sense on the input.

ro0gr commented 4 years ago

@Gaurav0 Thanks for example!

I agree, this seems to be the most prominent use case, when you want to use ...attributes in the nested element. As already mentioned, exposing the ...attributes is an internal implementation details. However, in case if expose it from nested element, like <input wrapped by something, that would be details not of the custom input component, but details of the HTMLInput. That can go wrong in many ways:

But that's not the only problem. Let's assume the following minimal example of such custom input implementation and usage:

{{#let "InputWithAttributes" as |scope|}}
  <div class={{scope}} ...attributes>

    {{#if @label}}
      <label for={{this.inputId}}>{{@label}}</label>
    {{/if}}

    <input
      ...attributes
      id={{this.inputId}}
      class="{{scope}}-input"
    >

    {{!-- error here --}}
  </div>
{{/let}}

An important detail is that I've put ...attributes twice. Otherwise, if put it into <input only, we would break the web component, cause there is no reliable way to style it anymore, since our component doesn't recognize any attributes passed. There is no any built-in HTML tag or CustomElement which wouldn't render an attribute you pass into it. That's why I say, ...attributes at the outer-most element is a must for any component.

Back to our InputWithAttributes. We can invoke it like that:

<InputWithAttributes
  @label="Label"
  {{on "input" this.updateValue}}
  value={{this.value}}
  id="1"
/>

and the output looks fine:

<div class="InputWithAttributes" id="1">
  <label for="ember170">Label</label>
  <input id="ember170" class="InputWithAttributes-input">
</div>

We've even managed input's id to have a priority over id passed from outside by tweaking ...attributes order on <input. However, now we have problem with modifiers. Once I edit input, it's being called twice for obvious reasons.

Now, if we try to pass a class

<InputWithAttributes
  @label="Label"
  {{on "input" this.updateValue}}
  value={{this.value}}
  class="App-input"
  id="1"
/>

A passed App-input would appear twice as well because of class merging behavior:

<div class="InputWithAttributes App-input" id="1">
  <label for="ember169">Label</label>
  <input class="App-input InputWithAttributes-input" id="ember169">
</div>

To be clear, it isn't related to classes merging behavior. If I pass any custom attribute which we haven't blacklisted explicitly, for instance data-test it would also appear twice.

I'd say this issue is similar to breaking ...attributes in the outer element. In both cases we make styling and testing much more tricky.

pzuraq commented 4 years ago

However, now we have problem with modifiers. Once I edit input, it's being called twice for obvious reasons.

I personally think that using modifiers with components is, if not an antipattern, definitely something that should not be the main API for interacting with any component, regardless of whether or not ...attributes is on the top element. We have arguments and actions for a reason - the main APIs for interacting with a component should always be arguments and actions.

The internal details of how event listeners are added and elements are handled should not be something that your external code cares about. Otherwise you've already broken encapsulation, and you're going to be facing refactoring hazards no matter what.

An important detail is that I've put ...attributes twice. Otherwise, if put it into <input only, we would break the web component, cause there is no reliable way to style it anymore, since our component doesn't recognize any attributes passed. There is no any built-in HTML tag or CustomElement which wouldn't render an attribute you pass into it. That's why I say, ...attributes at the outer-most element is a must for any component.

This seems to point toward a need for more flexibility with ...attributes, not less. In components like this, it may make sense to split the attributes such that specific attrs are applied to the input, and the rest are applied to the wrapping element, or vice-versa. I actually think while a few key elements like class would probably want to be applied to the wrapper, you would actually want to spread the remainder into the input directly, since it's the more customizable element.

ro0gr commented 4 years ago

regardless of whether or not ...attributes is on the top element

I think that's the main confusion point, which is probably a root cause of the ticket.

Even <br preserves any attributes passed in:

<br id="nice-one" wherever="whatever" />

<script>
  document.querySelector('nice-one');
  hr.getAttribute("wherever"); // "whatever" 
</script>

If we allow the component's root to ignore passed attributes, such a component would break my expactations about output it produces. That's not aligned with what I've learned about HTML for years.

it may make sense to split the attributes such that specific attrs are applied to the input, and the rest are applied to the wrapping element, or vice-versa. I actually think while a few key elements like class would probably want to be applied to the wrapper, you would actually want to spread the remainder into the input directly

I'm curious is there a reason to remove attrs from the root at all?

In general, after some more thinking about the input case, and discussions, I think I'm finally convinced about usefulness of picking specific attributes somewhere deep in the component, but please, not vice versa! 😄


That's probably not a topic of the ticket, but what about:

  <div class={{scope}} ...attributes>

    <input
      readonly
      autocomplete="off"
      ...attributes
      id={{this.inputId}}
      class="{{scope}}-input"
    >

With that syntax, to the element would be applied only those attrs which are listed before the ...attributes.

The rules are stupid simple:

upd: well, seems like these rules break the root element's ...attributes, though it still feels close to a solution to me 🤔

pzuraq commented 4 years ago

@ro0gr I don't think this line of thought is going to continue to be productive. I've tried to outlined a few times now that the design process that we went through to get it was careful, considered a lot of these possibilities, and ended up with a well thought out result. Moreover, it would be a massive breaking change, and I'm not seeing a compelling reason to make that breaking change personally.

I still think the existing syntax can be improved, and I think we should be spending time and effort focusing on improving it, without breaking changes. Perhaps in a future edition of Ember, several years from now, we can revisit the fundamentals, but it would again be a massive effort to change things now, and short of a critical flaw I don't think it will happen.

ro0gr commented 4 years ago

@pzuraq Sorry, maybe I miss smth.. which breaking change are you refering to? I don't intend to, and I highly appreciate ember team commitment for stability.

Just in case, in my last code snippet, there is an attempt to provide a backward compatible(I believe), extension of attributes syntax, which seems to be quite natural at the same to me.

ro0gr commented 4 years ago

By the way, I created a ticket related to discoverability https://github.com/emberjs/ember.js/issues/18799

If this implemented, you should not fall in a situation when you pass attributes, and they got lost, because of missing ...attributes.

pzuraq commented 4 years ago

Sorry, maybe I miss smth.. which breaking change are you refering to?

Attributes already have well-defined semantics for merging properties. If you spread ...attributes after some other listed attributes, then it will merge the values, with any values provided by ...attributes winning. So, attempting to use that syntax would be a breaking change, because many components are already relying on this behavior.

I think the best avenue to explore would be some sort of syntax for accessing attribute values directly in templates. I'm not sure what that would look like, maybe something along the lines of:

  <div class={{@@class}}>

It would definitely require a unique sigil. I could also imagine that attributes becomes a keyword in strict mode templates, so you can reference it directly and it doesn't do a fallback lookup to the component instance:

  <div class={{attributes.class}}>

Also, going back to an earlier comment:

If we allow the component's root to ignore passed attributes, such a component would break my expactations about output it produces. That's not aligned with what I've learned about HTML for years.

I think this more be out of line with we've learned from Ember for years. Web components, which are the browser's built in way of achieving this functionality, have no such guarantees. You can receive attributes however you like, and do whatever you want with them. You can apply them to the root element, or a sub element, and so on. So I think this may just be learning a new set of norms.

ro0gr commented 4 years ago

Web components, which are the browser's built in way of achieving this functionality, have no such guarantees.

I believe, with customElements any passed attribute is rendered by default until you explicitly removeAttribute(. Not an expert in WebComponents though.

If you spread ...attributes after some other listed attributes, then it will merge the values, with any values provided by ...attributes winning.

This part of semantics was not supposed to be changed. It should work the same, but in a slightly more smart way. However, there are some edge cases... I'll think about it more and continue in https://github.com/emberjs/rfcs/issues/503, if come up with something thought well enough.

gossi commented 4 years ago

What I've discovered, is that often arguments are abused to compensate the lack of discoverability of attributes being suggested to use. Common arguments I see are @label, @id or @accessibility* ones.

That is even more pleasing if you use e.g. UELS and you have intellisense at your fingertips for arguments. However, it is still wrong! For example:

<MyComponent @accessibilityLabel="Hello"/>

vs

<MyComponent aria-label="Hello"/>

The second form is not only shorter it is also the HTML first approach. However, especially for components, it is unclear what kind of attributes are good to pass in. I think mostly affected are aria-* attributes in this scenario and components try to be clever by taking them as arguments puttin them on the respective elements within but don't provide a good enough API for you to do it on your own.

By API I mean the right element structure within your component, yielded components or suggestion of what elements could go inside your components and what attributes are suited on them.

Since there are plenty possible attributes available, I think the author of a component needs a way to suggest which ones he intended to use in here. With also special treatments to aria-* so this is directly addressed when designing a component.

lupestro commented 4 years ago

The abstraction of a labeled input field is valid and can be represented more than one way. For the abstraction, you need to know there is the idea of a label and the idea of an input, but can still supply attributes without more precision than that.

It would be most useful if one could supply named "splattribute sets" as parameters to a component, so that we can say "these apply to the label" and "these apply to the input" without talking about precisely how the HTML inside is arranged.

And, yes, it wouldn't surprise me at all if a discussion of this idea ended up running parallel to the named-blocks discussion.

gossi commented 4 years ago

@lupestro what you are aiming at is a way where to put ...attributes or named attributes. This issue is about describing a collection of attributes and where they possibly end up.

Here are two stupid ideas to start with:

interface AttributesTarget {
    element: string | HTMLElement;

    /* If the ...attributes are attached to multiple elements (if multiple, then `id` is bad) */
    multiple: boolean;

    suggestions: Record<string, string>;
}
interface AttributesTarget extends HTMLDivElement {
    multiple: boolean;
    suggestions: Record<string, string>;
}

Notes:

NullVoxPopuli commented 2 years ago

There is some good discussion in here. I'm going to close the issue though as the Glimmer Signature RFC has been accepted, merged, and implemented, and covers most of what we wanted here :partying_face: