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

Emitting v-model event leads to unexpectedly updated prop value #440

Closed
nekosaur opened this issue Mar 3, 2021 · 7 comments · Fixed by #459
Closed

Emitting v-model event leads to unexpectedly updated prop value #440

nekosaur opened this issue Mar 3, 2021 · 7 comments · Fixed by #459

Comments

@nekosaur
Copy link
Contributor

nekosaur commented Mar 3, 2021

I have a scenario where I want to test that interacting with a component (that results in emitting a v-model event) does not lead to a state change in the internals of a component.

Here is a simplified version of what I'm trying to do

import { mount } from '@vue/test-utils'
import { defineComponent, h } from 'vue'

const Foo = defineComponent({
  name: 'Foo',
  props: {
    foo: String,
  },
  emits: ['update:foo'],
  setup (props, ctx) {
    return () => h('div', {
      onClick: () => {
        ctx.emit('update:foo', 'baz')
      },
    }, props.foo)
  }
})

test('something', async () => {
  const wrapper = mount(Foo, {
    props: {
      foo: 'bar',
    }
  })

  expect(wrapper.text()).toEqual('bar')

  await wrapper.trigger('click')

  expect(wrapper.text()).toEqual('bar')
})

Currently this fails on the second expect with a received value of baz. This to me feels very strange and unintuitive. The component has emitted an event, but I have done nothing to catch it and subsequently update state to trigger a re-render, as I need to do in a real Vue app. Yet the props have been modified.

The change was introduced in this PR #393. With this change, components under test do not behave in the same way as in normal operation.

edit:

Something a bit closer to real usage would be if you could perhaps do something like this, if you need the 2-way binding to be automatic.

const wrapper = mount(Foo, {
  props: {
    foo: 'bar',
    onUpdateFoo: async (foo) => {
      await wrapper.setProps({ foo })
    }
  }
})

Replacing the code from the PR with the following works, but I've no idea how stable it is.

var eventFnName = `on${event.split(':').map(vue.capitalize).join('')}`
if (wrapperVm.attrs[eventFnName]) {
  wrapperVm.attrs[eventFnName](...args)
}
@lmiller1990
Copy link
Member

lmiller1990 commented Mar 4, 2021

Good point... there (is?) was a discussion around this where people want the opposite to happen. I think it probably led to the PR you linked.

Regarding "... With this change, components under test do not behave in the same way as in normal operation ..." is this true? In most situations emitting the update event will automatically update the v-model bound value, won't it? No need to do any catching/handling.

Either way, it seems like some users would like update events to update the value (thus the discussion and subsequent PR) and others would not (like you described). I am not sure how to proceed, I wonder if we can find some way to have a win-win where everyone is happy. Any ideas?

@nekosaur
Copy link
Contributor Author

nekosaur commented Mar 4, 2021

If you are using the v-model sugar, then yes it will of course update it 'automatically', but that is not what is happening in my scenario, or the test case from the PR. They are both analogous to only using :value, which will not automatically update the prop when event is emitted.

I always prefer being explicit, especially when it comes to tests, so I like my suggestion (of course :)). But I realize not everyone is me and that it might seem like unnecessary boilerplate code.

From the discussion you linked I also like the vmodel: {} option. Passing a ref to props a little bit less so, because it might not be immediately obvious that it will lead to an updated value (again comparing it to regular usage).

@nekosaur
Copy link
Contributor Author

nekosaur commented Mar 4, 2021

vModel could also be a simple string array, that specifies which props should be treated as 2-way bound values.

const wrapper = mount(Foo, {
  props: {
    one: 'hello',
    two: 'world',
  },
  vModel: ['one'],
})

Internally test-utils would add onUpdateOne listener when rendering the mounted component, that updates prop value.

@nandi95
Copy link
Contributor

nandi95 commented Mar 9, 2021

@nekosaur So if I understand right your component is like <component @update:value="customValueUpdater" :value="value" />?

I think having a v-model is more frequent therefore we should flip the logic and instead of vModel we could do oneWay?

While this discussion is resolved you could still do the customValueUpdater by checking the last emitted event I believe (not ideal).

@nekosaur
Copy link
Contributor Author

nekosaur commented Mar 9, 2021

My specific use case is probably not a very common one, it only served to highlight the actual issue for me.

These two markups have very different behaviors in Vue, right. The first one will only ever pass down a value from outside the component, while the second one will update the outside value with a new one from inside the component.

<component :value="value" />
<component :value="value" @update:value="value = $event" />

The current test-utils behavior does not match this. Mounting the first example and emitting an update:value event from inside the component will update the value prop value. To me this is very unexpected.

This is what I would consider to be the corresponding test-utils mounts.

mount(Component, {
  props: {
    value
  }
})
mount(Component, {
  props: {
    value,
    onUpdateValue
  }
})

In Vue you have to explicitly opt in to 2-way prop bindings with either v-model sugar or prop and event handler. I think the same should hold true for test-utils.

@nandi95
Copy link
Contributor

nandi95 commented Mar 9, 2021

Right, I think you're right. But given vue gives the syntactic sugar of v-model perhaps vtu can offer something similar?
I'm thinking a global config autoMockPropListeners: boolean or something alonge those lines? This would save us from writing boilerplate code.

Apart from that I agree however, I think onUpdateValue should go into its own listeners: {} or something as it isn't really a prop.

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 12, 2021

I am not sure adding more config to reduce boilerplate by a few lines is really a good trade-off. Anyone can read boilerplate, but more configuration options leads to more complexity.

This doesn't mean it's a bad idea; I am just not 100% sold on more features when you might be able to accomplish the same thing with a few lines (or a util function).

I think we probably need to revert to the original behavior, since that is how v1 works, and we want to have as few breaking changes as possible. As for v-model sugar, we should try to find the most ergonomic and intuitive API. Another alternative is just recommend what KaelWD shows here.

nekosaur added a commit to nekosaur/vue-test-utils-next that referenced this issue Mar 13, 2021
nekosaur added a commit to nekosaur/vue-test-utils-next that referenced this issue Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants