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

Simplifying the transparent wrapper pattern #9

Closed
chrisvfritz opened this issue Oct 13, 2018 · 11 comments
Closed

Simplifying the transparent wrapper pattern #9

chrisvfritz opened this issue Oct 13, 2018 · 11 comments

Comments

@chrisvfritz
Copy link
Contributor

The transparent wrapper pattern is very convenient and increasingly common, but still a little clunky in Vue 2.x. For example, to get a BaseInput component that can be used exactly like a normal input element, we need to do this:

<script>
export default {
  props: {
    value: {
      type: String
    }
  },
  computed: {
    listeners() {
      return {
        ...this.$listeners,
        input: event => {
          this.$emit('input', event.target.value)
        }
      }
    }
  }
}
</script>

<template>
  <input
    :value="value"
    v-on="listeners"
  >
</template>

In Vue 3, that will currently translate to:

<script>
class InputText extends Component {
  // Currently, `v-model` on a component assumes `value` is a 
  // prop, so it will never be included in $attrs, even if no
  // `value` prop is defined. This forces users to always define 
  // `value` as a prop, even if it's never needed by the component.
  static props = {
    value: String
  }

  get attrs () {
    return {
      ...this.$attrs,
      // We have to manually include the `value` prop to `attrs`,
      // due to `v-model`'s assumption that `value` is a prop.
      value: this.value,
      // Since `v-model` listens to the `input` event by default, 
      // the user is forced to override it when using binding 
      // $attrs in order to prevent the unwanted behavior of the
      // `event` object being emitted as the new value.
      onInput: event => {
        this.$emit('input', event.target.value)
      }
    }
  }
}
</script>

<template>
  <input v-bind="attrs">
</template>

Still quite a bit of work. 😕

However, if we remove v-model's assumption that value is a prop, then it no longer has to be added separately. And if we also make v-model listen for an update:value event by default (as @posva suggested here), instead of input, then we no longer have to override input and can just add a new listener.

With these 2 changes, our BaseInput component would simplify to:

<template>
  <input
    v-bind="$attrs"
    @input="$emit('update:value', $event.target.value)"
  >
</template>

To me, this is much more convenient and intuitive. 🙂

@yyx990803 @posva @johnleider What do you think?

@yyx990803
Copy link
Member

yyx990803 commented Oct 14, 2018

Actually with v3's flat vnode data implementation, declaring props is unnecessary. When you pass data down it doesn't matter if it's a prop or not.

So the wrapper can simply do

<input v-bind="$attrs">

Regarding the input event re-emit, I kinda want to avoid this event vs. value mismatch altogether. Maybe the code v-model generates should just handle both event and plain values. e.g. value = value instanceof Event ? value.target.value : value

@chrisvfritz
Copy link
Contributor Author

Actually with v3's flat vnode data implementation, declaring props is unnecessary. When you pass data down it doesn't matter if it's a prop or not.

Yay! 😄

Regarding the input event re-emit, I kinda want to avoid this event vs. value mismatch altogether. Maybe the code v-model generates should just handle both event and plain values. e.g. value = value instanceof Event ? value.target.value : value

I think that'd be great, but I still feel like the default event for components should be update:value. It's not only more consistent with other 2-way bindings, but also feels more semantically accurate for a component, since updates may be triggered by things other than "input" from the user (e.g. responses from an API, images loading, etc).

Making v-model smarter with components

One other thing I think we need to really improve v-model with components is detecting the kind of element that arbitrary attributes are passed through to. That way, v-model can be as smart with components as it is with normal elements and always "just work", using the correct attribute and event. (This would also eliminate the need for the model option on components.)

Otherwise, I worry it might be confusing if, depending on the element, v-bind="$attrs" sometimes just works (e.g. <input type="text">), but often doesn't work (e.g. <select>, <input type="checkbox">, etc). In my experience, most developers don't know the exact attributes and events necessary to work with every form element, so offering the same convenience for components that we do for elements would save a lot of development hours.

@yyx990803
Copy link
Member

yyx990803 commented Oct 15, 2018

@chrisvfritz yeah, I agree regarding emitting update:value instead. #8 also makes sense.

Making v-model smart can be tricky, because we can't know at compile time what element the $attrs will be spread on in the child component.

One possible implementation: v-model generates code like this:

<input v-model="foo">
h('input', {
  modelValue: this.foo,
  'onUpdate:modelValue': value => {
    this.foo = value
  }
})

When used on a component:

  • A component that expects to work with v-model should expect a prop modelValue
  • To trigger update, it should emit an event named update:modelValue

When used on an element, the runtime will dynamically transform modelValue and update:modelValue to appropriate values depending on target element type when patching the element:

  • modelValue -> value or checked based on element type
  • update:modelValue -> input or change based on element type, also handles extracting the value from raw DOM event.

This way, v-model will generate the exact same code when used on elements AND components. A component with v-model can simply spread $attrs on any element and it will just work.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Oct 15, 2018

@yyx990803 Brilliant! I love it. 😻 A few follow-up questions:

  • Should v-model="foo" actually translate to an onModelUpdate:value event (instead of onModelUpdate:foo)?

  • For consistency, should v-model:my-prop="foo" translate to modelMyProp and onModelUpdate:myProp, which could also be transformed at runtime to the correct attribute and event?

  • Should registering myProp as a prop also automatically register:

    • modelMyProp
    • onModelUpdate:myProp
    • onUpdate:myProp


    That way, all of these would automatically stay out of $attrs. Then modelMyProp and onModelUpdate:myProp could just be aliases for myProp and onUpdate:myProp, but with special behavior when bound to an element. Would that make sense?

With those final tweaks, I think we might actually have the perfect v-model API!

  • The simplest cases remain as simple as possible: just v-model="foo" on an element, with v-bind="$attrs" added for a component.

  • In cases where users want to manage v-model behavior for a prop manually, they can just use myProp and update:myProp without having to worry about the special model/onModel-prefixed attributes.

  • When users need full access to v-model's magic on an element, they can choose to explicitly use modelMyProp and/or onModelUpdate:myProp to achieve the exact behavior they're looking for, with minimal boilerplate.

@yyx990803
Copy link
Member

yyx990803 commented Oct 15, 2018

I just edited my proposal a bit. When used without an argument the generated props should not care about what key it is.

When used without an argument:

<input v-model="foo">
h('input', {
  modelValue: this.foo,
  'onUpdate:modelValue': value => {
    this.foo = value
  }
})

When used with an argument it will compile like this:

<Comp v-model:someProp="foo">
h(Comp, {
  someProp: this.foo,
  'onUpdate:someProp': value => {
    this.foo = value
  }
})

So someProp is passed down with value bar, and the child can emit update:someProp to perform 2-way sync.

I don't think it makes sense to use v-model with arguments on a plain element, it seems the use case is really for two-way binding on component only. So the runtime special handling will only apply to modelValue and onUpdate:modelValue.

@chrisvfritz
Copy link
Contributor Author

I don't think it makes sense to use v-model with arguments on a plain element, it seems the use case is really for two-way binding on component only. So the runtime special handling will only apply to modelValue and onUpdate:modelValue.

@yyx990803 I agree it doesn't make sense to use v-model with arguments on plain elements, but I was thinking about cases where a user might still want to bind props to individual elements within a component, e.g.:

<InviteeForm
  v-model:name="inviteeName"
  v-model:email="inviteeEmail"
/>

where InviteeForm is defined as:

<script>
export default class InviteeForm extends Component {
  static props = ['name', 'email']
} 
</script>

<template>
  <form v-bind="$attrs">
    Name: <input v-bind="{ modelName, 'onModelUpdate:name' }">
    Email: <input v-bind="{ modelEmail, 'onModelUpdate:email' }">
  </form>
</template>

Though maybe we could actually abstract away the implementation details of:

<input v-bind="{ modelName, 'onModelUpdate:name' }">

by simplifying to something like:

<input v-model.prop="name"> 

What do you think?

@Jinjiang
Copy link
Member

Sorry. Why not use value directly but choose modelValue by default? Did I miss something?

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Nov 30, 2018

Why not use value directly but choose modelValue by default?

@Jinjiang I don't think I understand the question. Could you elaborate?

@Jinjiang
Copy link
Member

Jinjiang commented Nov 30, 2018

Sorry for my bad description. 😅

As we discussed above, now

<input v-model="foo">

will equal to:

h('input', {
  modelValue: this.foo,
  'onUpdate:modelValue': value => {
    this.foo = value
  }
})

not:

h('input', {
  value: this.foo,
  'onUpdate:value': value => {
    this.foo = value // or `value = value instanceof Event ? value.target.value : value`
  }
})

right? So my question is why we choose the name modalValue but not value here?

@chrisvfritz
Copy link
Contributor Author

@Jinjiang Ah, I see. If I understand correctly, it's because modelValue has special behavior specific to the element it's added to. For example, on a <select> element it will not map to a value attribute, but instead add selected to the appropriate <option>. Does that make sense?

@yyx990803
Copy link
Member

Closing in favor of vuejs/rfcs#8 and vuejs/rfcs#31

@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants