Skip to content
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

Attr fallthrough behavior #26

Closed
wants to merge 5 commits into from
Closed

Attr fallthrough behavior #26

wants to merge 5 commits into from

Conversation

yyx990803
Copy link
Member

  • Disable implicit attribute fall-through to child component root element

  • Remove inheritAttrs option

Rendered

@yyx990803 yyx990803 added 3.x This RFC only targets 3.0 and above breaking change This RFC contains breaking changes or deprecations of old API. core labels Apr 8, 2019
@alexsasharegan
Copy link

Could we get clarification on how best to reproduce the current class/style attr merging in the 3.x api when these conditions exist:

  • the component already has a class/style applied to it's root node (just 1 in this case)
  • the component wants to also spread the other attrs (to support things like aria as mentioned), but declares a few of its own that it doesn't want over-written

I think for myself and many others with a lot of JSX experience, we know there are gotchas for exactly where you spread attrs and the desired behavior--spreading before defining your own attrs ensures they aren't over-written, while spreading after has the effect of providing default attr values.

@yyx990803
Copy link
Member Author

@alexsasharegan that's a good question. In templates v-bind="$attrs" handles style/class merging for you. Like you mentioned, we should make sure the overwriting behavior is determined by its position relative to other existing attributes. So if you don't want certain attributes to be overwritten, place them after the v-bind:

<div v-bind="$attrs" aria-hidden="true"></div>

In render functions, Vue will expose a mergeData helper (see updated RFC) that performs proper merging. Our JSX plugin will also compile spread props into mergeData calls so that style/class merging is handled properly:

<div { ...this.$attrs } aria-hidden="true"/>

Both the template and JSX examples above compiles into:

return h('div', mergeData(this.$attrs, { 'aria-hidden': true }))

@Justineo
Copy link
Member

Justineo commented Apr 9, 2019

Now in 2.x, single bindings always overwrite the v-bind object and I think it's quite straightforward. Not sure if relying on attribute order is a good idea (applying JavaScript semantics in HTML scope).

@alexsasharegan
Copy link

As the author of vue-functional-data-merge, I like the sound of that!

@Justineo

This comment has been minimized.

@mika76
Copy link

mika76 commented Apr 11, 2019

we should make sure the overwriting behavior is determined by its position relative to other existing attributes.

This doesn't sound so great to me - it's hidden from anybody who hasn't read docs in great detail. I don't think Vue has anything else like it. Isn't there a more declarative way? Adding a modifier or something?

[EDIT]
Maybe something like <div v-bind.dontInheritAfter="$attrs" aria-hidden="true"></div> where inheriting all would be default.

or what about a prefix like ! on the attribute !aria-hidden="true" - not sure if it's allowed by HTML
but an exclamation is usually understood as NOT

@posva
Copy link
Member

posva commented Apr 11, 2019

@Justineo I think that's working normally because in the case of inputs, the value attribute is used during the first render but then v-bind is used by Vue to change the value, whereas, with components, the attribute value takes the upper hand.

If you use a v-bind on input, it shows off consistent

@alexsasharegan
Copy link

alexsasharegan commented Apr 11, 2019

@mika76, this kind of stuff gets into the weeds of the underlying abstraction--the virtual dom. Component attributes are described by objects, so when you have to merge attribute objects from multiple sources, the simplest tool is Object.assign, which is sensitive to position such that the last source to provide a key will set that key's final value in the resulting object (argument[0], the target of Object.assign).

Vue has been really friendly and helpful when it comes to merging attributes, especially given the deep merging of things like class & style. I understand that class/style merging will continue, and anyone getting into the weeds writing function components will now have access to those merge behaviors by way of an exposed helper function.

I agree that this whole area could benefit from additional detail in the docs. One thing to consider, what assumptions do you have about what would happen currently if you saw a component written with duplicate attributes like this (disabled is duplicated):

<x-button disabled type="button" :disabled="false">
  Example Component
</x-button>

@Justineo
Copy link
Member

If you use a v-bind on input, it shows off consistent

@posva Yeah you are right! I prefer this behavior than relying on the order of attributes in templates.

@dima74
Copy link

dima74 commented Apr 12, 2019

As a developer who worked both in vue and react projects, I strongly dislike disabling fall-through for class and style. For me it was always painful to style child components in react, because I had to create unnecessary wrappers (which are ugly and also are bad from both semantically and performance point of view) and use tedious helpers for merging classes. And I always was exciting about how elegant vue allows to style child components (thanks to fall-through behaviour).

@alexsasharegan
Copy link

Maybe we can get some clarification, @dima74, because my assumption was that class/style merging would continue except when you author your own render functions in function components. I think this does get confusing when you consider there are many ways to describe how a component should render:

  • Instance Component
    • using a dom template
    • using a string template
    • .vue <template>
    • render method
      • createElement calls
      • JSX vue dialect
  • function component
    • <template functional>
    • createElement calls
    • JSX vue dialect

And maybe more (are dom/string templates okay for function components?).

@posva
Copy link
Member

posva commented Apr 12, 2019

it's the same fall through behaviour except now it has to be explicit. This is to make it consistent with Fragments components (more than one element at the root), which wasn't possible before. You won't need any wrappers

@sdras
Copy link
Member

sdras commented Apr 17, 2019

In class and style bindings that are dependent on a parent/child relationship such as position: relative on a parent and position: absolute on child to place it on a containing unit, this becomes a bit more messy. There are workarounds but I'm concerned that this impedes the ability to position things without some annoyances. Happy to talk through it if anyone would like clarification.

@casey6
Copy link

casey6 commented Apr 23, 2019

So to enable the 2.x functionality you need to add v-bind="$attrs" to every single component in your application? That feels pretty inconvenient.

I understand the argument that it encourages a more "correct" way of building components. But in the real world people aren't perfect and I'd argue using style/class for child component styling is a great way to achieve your goals in the absence of perfection.

I'd be interested to see how many people currently write code like <my-component class="some-class"/> for styling purposes. I suspect the number is extremely high.

@LinusBorg
Copy link
Member

LinusBorg commented May 20, 2019

I came across one blind spot with this concept: Event modifiers.

We currently provide a couple of handy modifiers such as .prevent for native event listeners:

<a @click.prevent="handleLinkClick">

Some of these modifiers (i.e. .prevent) interact with the event object that the native event passes to the listener, i.e. to call event.preventDefault(). These modifiers don't work for component events, obviuosly, and aren't meant to.

In Vue 2, if we wanted to listen to a native event and prevent its default behaviour, we could do this:

<fancy-a @click.native.prevent="handleLinkClick">

However, this RFC drops support for .native. Instead, the child component is now in full control over where to attach any and all v-on listeners that were defined for it by the parent. Converning modifiers, this seems to create leaky abstractions - the consumer of our component has to be aware how an event listener would be used in the child in order to determine wether using .native would work or not.

<fancy-a @click.prevent="handleLinkClick">

<!-- Possible Implementations in FancyA: -->

<a v-bind="$attrs"> <!-- WORKS -->

<!-- OR -->

<a v-on:click.prevent="prepareStuff">
<script>
  export default {
    methods: {
      prepareStuff(e) {
        // do something else, then:
        this.$emit('click', e.target.value) // BREAKS, argument is not an event object
      }
    } 
  }
</script>

Edit: Now that I think about it, that shouldn't be a real issue.

  1. Events that the component explicitly handles would ideally be documented, so people know if they get a raw event back or not.
  2. Non-documented listeners would generally not be handled by the component at all, so they would always get an event object.

Would work even better with something like #16

@aminimalanimal
Copy link

aminimalanimal commented May 27, 2019

I like the improvements this makes for v-model and passing properties to form elements, but I'm alarmed about the proposed changes to fallthrough classes and styles.

I use fallthrough classes to handle making adjustments to the layout or positioning of child elements given circumstances that the child components need not be aware of.

The documentation attached to this RFCS states the following (emphasis mine):

This may cause some inconvenience for users accustomed to the old behavior, especially when using class and style for styling purposes, but it is the more "correct" behavior when it comes to component responsibilities and boundaries.

I think we can all agree on the following rules for the “correct” behavior for component responsibilities and boundaries (regarding styling):

  • components should handle their own appearance
  • components should handle their own “internal” layout
  • a parent component shouldn't target any class specified by a child component
  • parent components should handle in-context/situational “outer” layout, positioning, and sizing of child components

The beauty of Vue 2's behavior is that if a parent component needs to style a child component's root element directly:

  • the override stays entirely in one file—in its proper context
  • no additional containers are added to the DOM
  • no semantics are ever impacted
  • the child component does not need to be explicitly set up to play nice
  • the API for adding the class is consistent (at least within templates)

With the proposed changes, third-party components would have to allow your parent components to pass a class onto them, which from a component's contractual standpoint is, I suppose, more “correct,” but I really struggle to see any way in which this limitation is an improvement to the developer experience since the API for adding a class to the root element would no longer be consistent.

  • it may not be present at all
  • it may be present, but implemented in various ways
    • best-case $attrs method: if $attrs needs be applied to a form element for v-model support, a developer could presumably create a computed property to omit the class/style portions from the form element, and apply only the class/style portions to the root element; otherwise, just place $attrs on the root element
    • clumsy $attrs method: $attrs is just dumped in both spots entirely, causing an internal element to gain a class you probably only wanted on the root
    • prop method: set up a prop for a custom class, forcing the parent component to adhere to a non-standard prop name (and pug users miss out on the syntactical sugar of just sticking it on as MyComponent.my-class, but oh well...)

I really think that the current fallthrough capability for classes and styles is important to maintain.

There may be use-cases where a parent component needs to add/remove/adjust a component's default margin, height, width, position, directional properties, overflow, (or, and I hesitate to say this—float)... As @sdras pointed out, depending on the context, a wrapper component is not always sufficient for handling these things... and there are certainly plenty of situations I've ended up in where a flatter DOM structure was better or even necessary to accomplish the styling task at hand.

To @casey6 's point, I use fallthrough classes in almost every parent/child relationship, so if this behavior ships in Vue 3, I'm probably going to have to/want to implement an explicit $attrs solution in practically every component I make.

As @dima74 mentioned, having to wire this stuff up manually in React was painful... Vue 3 would still be better than that thanks to class merging, but not better than Vue 2 due to the lack of consistent application. I think that the inconvenience of having to manually wire this up is likely to cause developers to add styles that impact outer-layout directly to their components.

Overall, I think that class and style should be treated differently from other properties in this regard, maintaining their current functionality of being placed on the root element, and that there should be a version of $attrs that excludes them for the purpose of solving the v-model/property issue for forms that this RFCS addresses.

In cases where there are multiple root elements, I would want my fallthrough class applied to each of them. (But if that isn't possible, I'd rather have the inconsistency than the proposed inconveniences, as instances in which I need fragments are fairly rare (dd/dt)).

This is one of the bits of functionality that I think makes Vue great, and makes it a cut above the rest. I hope it's possible to keep.

@tmorehouse
Copy link

What about using an options flag, that denotes that the component renders (or could render) more than one root element?

It many cases, adding an outer wrapper for styling positioning adjustments can break layout for some CSS libraries that expect a certain element structure, specifically CSS sibling selectors (when components are children of a parent element).

How would consumers know if a component will merge style/classes on it's root element?

@yyx990803
Copy link
Member Author

After core team discussion I'm closing this since it is a major breaking change for a lot of use cases, and it's very difficult to provide a smooth upgrade path. We will be submitting a new RFC with a smaller scope of changes instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x This RFC only targets 3.0 and above breaking change This RFC contains breaking changes or deprecations of old API. core
Projects
None yet
Development

Successfully merging this pull request may close these issues.