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

Improved API for UI components, reducing boilerplate for forwarding attributes and events #5983

Closed
chrisvfritz opened this issue Jun 26, 2017 · 46 comments

Comments

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Jun 26, 2017

What problem does this feature solve?

There are many cases where attributes passed to a Vue component should not be added to the root element, but rather a sub-element. For example, in this UI component, an incredible amount of props must be used to ensure that attributes are added to the input element, instead of the wrapper div.

Additionally, it's often desirable to expose all event listeners on a form element to the parent, which also requires a lot of boilerplate currently if the element is not the root (in which case, the .native modifier can solve the problem).

What does the proposed API look like?

EDIT: Start here to catch up on the discussion.

Currently by default, the "exposed" element (the one that arbitrary attributes can be added to) is always the root element. A new directive could be used to define a different exposed element. Some ideas for the name of the directive:

  • v-expose (probably my personal favorite)
  • v-expose-attrs (probably clearer, but lengthier)
  • v-main
  • v-primary

If v-expose is added to an element, it will accept attributes passed to its component - and these attributes will no longer be passed to the root element.

Other features that may be nice:

  • If the directive is defined on multiple elements, attributes will be duplicated across each of them
  • In the case where a subset of attributes should be accepted by an element, v-expose could accept a string or array of strings (e.g. v-expose="class" or v-expose="['class', 'type', 'placeholder']"). In this case, these attributes would be added to the element (again, instead of to the root element), but all other attributes would be added to the root element or to the element(s) with a valueless v-expose.
@nickmessing
Copy link
Member

Hmm, I don't know about that but for components like that I think you could use JSX or createElement to spread props.

@Austio
Copy link

Austio commented Jun 26, 2017

https://github.com/doximity/vue-genome

For us this would be a great. We wrap all inputs in a label for styling and ux purposes. I agree that we could drop down to jsx instead but the templates are so much easier for everyone to follow.

@nickmessing
Copy link
Member

@Austio, unfortunately, that's the payback for templates... wait... maybe we could think of a way to spread props into vue templates?

@jkzing
Copy link
Member

jkzing commented Jun 27, 2017

I like this feature personally. But it seems to break the consistency of the v-bind behavior, like sometimes I still need to bind class property for the root element.

So how about use a pair of directives as getter and setter like:

Inside the component, define a v-expose anchor:

<input v-expose="foo" />

When using it:

<the-component v-define:foo="{propA: '', propB: ''}"></the-component>

<!-- or maybe use v-bind for it directly -->
<the-component :foo="{propA: '', propB: ''}"></the-component>

@nickmessing
Copy link
Member

nickmessing commented Jun 27, 2017

@jkzing, that looks awesome, but again, that looks like a basic spread and with potential problems like how would you define @keyup.enter.prevent="myAction"?

@nickmessing
Copy link
Member

you can't just <the-component :foo="{'@keyup.enter.prevent': myAction}"></the-component>, that means you'd have to keep all modifiers like enter and prevent in the runtime (which is a part of vue-template-compiler atm)

@jkzing
Copy link
Member

jkzing commented Jun 27, 2017

@nickmessing

that looks like a basic spread

The thing we are talking about it is to bring something like spread for template users

<the-component :foo="{'@keyup.enter.prevent': myAction}"></the-component>

@ is a v-on shortland, doesn't means prop (v-bind).

@nickmessing
Copy link
Member

@jkzing, in the link from the description there's a lot of v-on bindings too

@jkzing
Copy link
Member

jkzing commented Jun 27, 2017

@nickmessing Um...As for v-on bindings, it is another topic IMO, like event bubbling. 🤔

@nickmessing
Copy link
Member

@jkzing, that was the whole concept of v-expose afaik, to make all properties "go" to a certain element in the component

@jkzing
Copy link
Member

jkzing commented Jun 27, 2017

@nickmessing , Can't be sure about the original proposal, but I don't think an event listener should be considered as attribute.

@nickmessing
Copy link
Member

@jkzing, probably not, but considering the common example of <my-awesome-text-input /> where you can have >9000 different props, you just want them all to get to yout <input /> that's inside your custom component without a ton of code.

@cristijora
Copy link

cristijora commented Jun 27, 2017

I personally use v-bind="$props" or you can filter those out to exclude the props you don't want to apply. This way you can apply multiple props at once on an input. Indeed v-expose might be useful because for wrapper components such as inputs you have to specify all those html props

So this
https://github.com/almino/semantic-ui-vue2/blob/master/src/elements/Input.vue#L9
cane be reduced to v-bind="$props" or v-bind="filteredProps" where filteredProps might be some computed property

@wojciechczerniak
Copy link

@cristijora We're using v-bind="someProps" too. The problem with this solution is that excessive properties will be added as HTML attributes. It would be great if v-bind= could filter out all properties that are not accepted by component. With dynamic <component> we don't know which props to filter out in computed property. Although it's possible to extract options.props and use lodash._pick.

@posva
Copy link
Member

posva commented Jun 27, 2017

Is this really feasible with a directive?

@nickmessing
Copy link
Member

nickmessing commented Jun 27, 2017

@posva, I don't think this will work as a directive per se, but that can be a part of vue template engine that does something like spread internally + some event propagation

@chrisvfritz
Copy link
Contributor Author

@posva Not a user-built directive I don't think, so I might be using the wrong language. What I mean is just a "special attribute".

@Austio
Copy link

Austio commented Jun 28, 2017

@chrisvfritz do you have any thoughts on an API for how it would be used (specifying what to expose and how to add to the child)

I could see this being similar in use to provide/inject concept.

@chrisvfritz
Copy link
Contributor Author

@Austio I might not be understanding the question, but I provide some thoughts on the API in the original post.

@Austio
Copy link

Austio commented Jun 29, 2017

Hey Chris, meant additional thoughts on using similar to provide inject where you declare what is to be exposable in the parent and then using that in the child.

@chrisvfritz
Copy link
Contributor Author

Ah, I see. I'm not sure there's a need for that. Information can already be passed via props and slots - and even private properties on the parent can be accessed with this.$parent, though I think it's best to avoid that pattern.

@Austio Is there a particular use case you're thinking of?

@LinusBorg
Copy link
Member

@chrisvfritz how would that work in render functions?

I think maybe it would be better to:

  • provide an option to disable auto-inheritance of attributes for the root node
  • expose those attributes as $attributes, for example (naming tbd)
  • use v-bind to add them wherever you like, much like we already showed to do with $props:
v-bind="$attributes"

That would have the added benefit of working practically identical in JSX/render functions

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Jun 30, 2017

@LinusBorg I like the way you think. 😄 Your way is much more intuitive.

As a sidenote, I think with this API in place, the next major version of Vue could even remove attribute auto-inheritance altogether, so that cross-component communication could remain explicit on both sides.

@chrisvfritz chrisvfritz changed the title New v-expose directive to define the element(s) that accept attributes API to define the element(s) that accept attributes Jun 30, 2017
@LinusBorg
Copy link
Member

It would be possible to depreciate or remove this behaviour, yes.

If that's worth the possibly required changes on many components in libs etc. is to be decided and should be discussed with the community, especially UI collection authors.

A about the prob posed feature: this information is already available in functional components via context.data.attributes, so this feature would give basically the identical functionality to instance components.

@chrisvfritz
Copy link
Contributor Author

Yes, exactly. The main purpose I have in mind is to make work simpler for UI component authors (both 3rd-party and internal). There are currently a lot of cases where something like this is necessary:

<input
  v-bind:id="id"
  v-bind:accept="accept"
  v-bind:alt="alt"
  v-bind:autocomplete="autocomplete"
  v-bind:autofocus="autofocus"
  v-bind:checked="checked"
  v-bind:dirname="dirname"
  v-bind:disabled="disabled"
  v-bind:form="form"
  v-bind:formaction="formaction"
  v-bind:formenctype="formenctype"
  v-bind:formmethod="formmethod"
  v-bind:formnovalidate="formnovalidate"
  v-bind:formtarget="formtarget"
  v-bind:list="list"
  v-bind:max="max"
  v-bind:maxlength="maxlength"
  v-bind:min="min"
  v-bind:multiple="multiple"
  v-bind:name="name"
  v-bind:pattern="pattern"
  v-bind:placeholder="placeholder"
  v-bind:readonly="readonly"
  v-bind:required="required"
  v-bind:src="src"
  v-bind:step="step"
  v-bind:type="type"
  v-bind:value="value"
  v-on:keydown="emitKeyDown"
  v-on:keypress="emitKeyPress"
  v-on:keyup="emitKeyUp"
  v-on:mouseenter="emitMouseEnter"
  v-on:mouseover="emitMouseOver"
  v-on:mousemove="emitMouseMove"
  v-on:mousedown="emitMouseDown"
  v-on:mouseup="emitMouseUp"
  v-on:click="emitClick"
  v-on:dblclick="emitDoubleClick"
  v-on:wheel="emitWheel"
  v-on:mouseleave="emitMouseLeave"
  v-on:mouseout="emitMouseOut"
  v-on:pointerlockchange="emitPointerLockChange"
  v-on:pointerlockerror="emitPointerLockError"
  v-on:blur="emitBlur"
  v-on:change="emitChange($event.target.value)"
  v-on:contextmenu="emitContextMenu"
  v-on:focus="emitFocus"
  v-on:input="emitInput($event.target.value)"
  v-on:invalid="emitInvalid"
  v-on:reset="emitReset"
  v-on:search="emitSearch"
  v-on:select="emitSelect"
  v-on:submit="emitSubmit"
>

A new $attributes property could shorten it to this:

<input
  v-bind="$attributes"
  v-on:keydown="emitKeyDown"
  v-on:keypress="emitKeyPress"
  v-on:keyup="emitKeyUp"
  v-on:mouseenter="emitMouseEnter"
  v-on:mouseover="emitMouseOver"
  v-on:mousemove="emitMouseMove"
  v-on:mousedown="emitMouseDown"
  v-on:mouseup="emitMouseUp"
  v-on:click="emitClick"
  v-on:dblclick="emitDoubleClick"
  v-on:wheel="emitWheel"
  v-on:mouseleave="emitMouseLeave"
  v-on:mouseout="emitMouseOut"
  v-on:pointerlockchange="emitPointerLockChange"
  v-on:pointerlockerror="emitPointerLockError"
  v-on:blur="emitBlur"
  v-on:change="emitChange($event.target.value)"
  v-on:contextmenu="emitContextMenu"
  v-on:focus="emitFocus"
  v-on:input="emitInput($event.target.value)"
  v-on:invalid="emitInvalid"
  v-on:reset="emitReset"
  v-on:search="emitSearch"
  v-on:select="emitSelect"
  v-on:submit="emitSubmit"
>

Though then I suppose it'd still be nice to have some way of also exposing events. Maybe an empty v-on directive could forward all event listeners on the parent to this element?

<input
  v-bind="$attributes"
  v-on
>

Or if there do end up being multiple concerns we want to bundle, we might be back to something like v-expose:

<input v-expose>

This has turned into a broader discussion of how to simplify the building of UI components, rather than a specific feature request, so I'll relabel this issue. 🙂

@chrisvfritz chrisvfritz changed the title API to define the element(s) that accept attributes Improved API for UI components, reducing boilerplate for forwarding attributes and events Jun 30, 2017
@alexsasharegan
Copy link

I'm late to this topic, but I have some thoughts as well.

v-bind Current Solution & Disadvantages

First off, I already use and love the v-bind="propObject" feature (so powerful). For example, bootstrap-vue has an internal link component that gets used everywhere (buttons, navs, dropdown lists, etc.). The component pivots becoming a native anchor vs. a router link based href vs. to & presence of vm.$router, so there are quite a lot of properties to conditionally pass to each of these components.

Our solution was to put those props in a mixin and use v-bind="linkProps" with a computed object. This works great, but it's still a lot of overhead adding that mixin to all the other components using the link component.

v-expose Possibilities using v-bind

I personally like the concept of v-expose, and maybe it could work like default slot + named slots, and then use modifiers to access the named attribute slots.

The default attribute "slot" would always pass attributes to the component itself (no change), while other named targets could be specified by the component. Perhaps something like this:

<template>
  <my-component 
    <!-- Nothing new here -->
    v-bind="rootProps"
    <!-- This binds the `linkProps` object to the named attribute slot `link` -->
    v-bind.link="linkProps"
  />
</template>

Inside of MyComponent.vue:

<template>
  <div>
    <router-link v-expose="link" />
  </div>
</template>

Event Proxying

I don't have a lot to add here, except that .native is an amazingly powerful modifier. Solved a lot of problems for me. It seems largely unknown to Vue devs though (I see a good amount of UI lib issues that get solved by exposing devs to this feature). I placed a PR on the website to add further docs and search support in the site and potentially optimized for google search.

@LinusBorg
Copy link
Member

Coming from an argument about API surface in another issue, I must repeat that I'm not a fan of the v-expose idea. it introduces another "way things work", and doesn't work for JSX without also implementing something special there, etc.

One thing I respect about React folks is their commitment to a slim API and using the language's features as much as possible. In that spirit, re-using a pattern we already have for props for attributes seems much better than introducing another abstraction.

<my-input
  type="file"
  mode="dropdown"
>
<template>
  <div>
    <input v-bind="$attributes">
    <dropdown v-bind="{ ...$props, $attributes.type }"/>
  </div>
</template

@alexsasharegan
Copy link

alexsasharegan commented Jun 30, 2017

Ahh, I see what you're saying now. And I like it! Is this currently available? vm.$attributes would be the addition instead?

@alexsasharegan
Copy link

Re-reading your comments. I'm tracking now 👍

@LinusBorg
Copy link
Member

LinusBorg commented Jun 30, 2017

Yes, $attributes would be the addition.

Also, we would need an option to turn of the current default behaviour of applying attributes to the root element, like this:

<template>
  <div>
    <input v-bind="$attributes">
  </div>
</template>
<script>
  export default {
    applyComponentAttrsToRoot: false, // defaults to true, name is tbd, obviously
    data() {
    
    },
    // ... options and stuff
  }
</script>
This could then become a default setting in Vue 3.0 if we then decide to do this resulting in a breaking change.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Jun 30, 2017

@LinusBorg What are your thoughts on dealing with the events side of things? To follow the same strategy, I supposed we could also add a $listeners property, which might look like this:

{
  input: function () { /* ... */ },
  focus: function () { /* ... */ },
  // ...
}

Then perhaps v-on could accept an object, similar to v-bind. So we'd have:

<input v-bind="$attributes" v-on="$listeners">

One issue that I foresee is with input/change events, since v-model works slightly differently for components than it does for elements. I also don't know if we'd want both $listeners and $nativeListeners. I suppose if $listeners were available, then the .native modifier might be obsolete.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Jun 30, 2017

Also, regarding the applyComponentAttrsToRoot option, perhaps exposeRootEl would be a good name, which when set to false, could disable both automatically applied attributes and .native event forwarding?

It might also be nice to be able to disable this for the entire application via Vue.config, as well as for a single component.

@LinusBorg
Copy link
Member

LinusBorg commented Jun 30, 2017

I recently had a similar idea about $listeners - it's also avaliable on functional components via

context.data.listeners

So we would end up with $props, $attributes, $listenerswhich sounds fine for me.

There's also #5578 asking for v-on="{...}" object syntax like I used for $attributes, it would fit right in.

But I'm unsure about the .native modifier. To make this work with both component events and native listeners, the API would end up much more complicated, and the use is questionable, since a native event listener applied to the root element would still catch the desired event bubbling up, so it might mnot be necessary to assign it to a specific element in the template.

@yyx990803
Copy link
Member

In general, I'd say for low-level component libs, render functions should be preferred when templates are getting awkward to work with. But I agree that the following would be valuable:

  1. Disabling the default behavior of "auto applying bindings that are not found in props as attributes to the root element" (related problem: should this affect class and style bindings as well?)

  2. exposing an easier way to "inherit" external bindings on the component onto an inner element that isn't necessarily the root. Ideally with consistency between templates and render functions.

@spiregoon
Copy link

ia kie like vue ,simple tools

@alexsasharegan
Copy link

Just want to say the PR for this in v2.4 is excellent! 👍

@AlexandreBonaventure
Copy link

From releases note

Combining these allows us to simplify a component like this down into this:

<div>
  <input v-bind="$attrs" v-on="$listeners">
</div>

Seems nice but that's not quite true, since these kind of components are designed to work with v-model and as far as I know v-model on wrapping component is not working out the box. Is there any example of how to forward v-model from a wrapping component onto a input for example ?
The simplest way I've found:
https://jsfiddle.net/60xdxh0h/2/

Maybe with functional component working along template it would be more straightforward

@LinusBorg
Copy link
Member

these kind of components are designed to work with v-model and as far as I know v-model on wrapping component is not working out the box.

Why would you think that? v-model is just syntax sugar for a prop and an event listener, both will be in $attr/$props and thus can be easily passed on.

I suppose the only thing that would require knowledge of the child options would be if the child changed the model defaults, that's true.

But it would be possible to read those, depending on the circumstances.

@AlexandreBonaventure
Copy link

AlexandreBonaventure commented Jul 24, 2017

Sure it is a syntaxic sugar, but I mean it could be confusing to read

Combining these allows us to simplify a component like this down into this:

when actually based on the example https://github.com/almino/semantic-ui-vue2/blob/master/src/elements/Input.vue, you can't just pass listeners directly, to achieve the same control. ( eg: you have to use v-on:input="emitInput($event.target.value)" )

Anyways, this PR is valuable, good job!

@chrisvfritz
Copy link
Contributor Author

@AlexandreBonaventure This is because v-model works slightly differently on components than it does on elements. DOM events provide an event object to callbacks, while component events provide a value directly. The result is that v-model does work, but the bound value is the DOM's event object. 😕

I think you're correct that it would be desirable for v-model to work here, but I'm not sure where the best place to solve it would be. Some ideas:

Maybe a non-enumerable property could be added to $listeners (e.g. __$listeners__: true, to help v-on detect uses of v-on="$listeners". Then in cases where the $listeners object is passed to v-on, each listener could be wrapped:

function (event) {
  listener(event.target.value)
}

One downside is now we're throwing away data. If someone wants to access a keyCode, for example, they're out of luck. However, if modifiers were supported for v-on's object syntax, we could fix this by making .native disable the automatic wrapping behavior.

@yyx990803 @LinusBorg What are your thoughts on feasibility? Any edge cases I'm missing?

@LinusBorg
Copy link
Member

Oh I see, you are referring to v-model on rral. Form elements, I was thinking about it on components.

You cant/shouldnt use that on props anyway, with or without this PR. And in advanced apps, it's rather uncommon to use it (though achievable).

@chrisvfritz
Copy link
Contributor Author

@LinusBorg Just want to make sure we're on the same page. Given a CustomInput component with this template:

<div>
  <input v-bind="$attrs" v-on="$listeners">
<div>

You wouldn't expect the code below to work?

<CustomInput v-model="myValue" />

@LinusBorg
Copy link
Member

I would expect it to work - but the way I understood alexandre, he was referring to v-model on the element, not the component - which eventually only works with mutating local state.

@AlexandreBonaventure
Copy link

I was trying to say what @chrisvfritz explained in his latter post. (English not my native language sorry :))

@RobertBColton
Copy link

RobertBColton commented Jul 25, 2017

@LinusBorg the problem with doing this in the latest release is that it's still considered an anti-pattern and triggers a warning about mutating the state.

It's extremely useful to have the above working where the value property is something other than a string. Take for example a combo component where I am trying to use enums imported from my own library as values for select options:

<template>
    <select class="combo" v-model="value" v-on="$listeners"> 
      <option v-for="(item, key) in items" :value="item">{{key}}</option>
    </select>
</template>

<script>
export default {
	props: {
		items: {
			type: Object,
			required: true
		},

		value: {}
	}
}
</script>

This is an example of one of the lists I use for items in the parent:

			execList: {
				"None": ACT_EXEC_TYPES.NONE,
				"Function": ACT_EXEC_TYPES.FUNCTION,
				"Code": ACT_EXEC_TYPES.CODE
			}

And how I use the combo component:

<combo :items="execList" v-model="selectedAction.execType"/>

I've been trying to make this work for 2 days now and still getting really frustrated. The problem is that $event.target.value is always a string and not evaluated like it should be in :value.

@chrisvfritz
Copy link
Contributor Author

@LinusBorg @AlexandreBonaventure @RobertBColton I just opened an issue where we can focus future discussion of this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests