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

fix(v-model): transformModel should check if prop is defined and fall back to attrs if needed (fix #8430) #8442

Closed
wants to merge 5 commits into from

Conversation

dattn
Copy link

@dattn dattn commented Jul 2, 2018

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@@ -247,7 +247,12 @@ function installComponentHooks (data: VNodeData) {
function transformModel (options, data: any) {
const prop = (options.model && options.model.prop) || 'value'
const event = (options.model && options.model.event) || 'input'
;(data.props || (data.props = {}))[prop] = data.model.value
// check if prop is defined, if not, attrs will be used
if (options.props && options.props[prop]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be refactored to

const propOrAtrrKey = options.props && options.props[prop] ? 'props' : 'attrs'
;(data[propOrAttrKey]|| (data[propOrAttrKey] = {}))[prop] = data.model.value

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a unit test for this please?

@dattn
Copy link
Author

dattn commented Jul 2, 2018

It is the first time i did a unit test, is this ok ?

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move the test to model-component.spec.js?

describe('Options model', () => {
it('key should fallback to attrs if prop is not defined in component', () => {
const ChildComp = {
model: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can omit the model option in the component

template: '<child-comp v-model="value" ref="child-comp" />'
}).$mount()

expect(vm.$refs['child-comp'].$attrs.value).toEqual(vm.value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just name it with a one-word name, you can name it test instead of child-component

import Vue from 'vue'

describe('Options model', () => {
it('key should fallback to attrs if prop is not defined in component', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better title: adds value to $attrs if no prop is defined

@dattn
Copy link
Author

dattn commented Feb 7, 2019

I think, this has been resolved with #9330, which has the same changes as mine ?

@yyx990803
Copy link
Member

@dattn ah yes, sorry for missing this earlier. Also note this actually caused some Vuetify components to break and was adjusted in b034abf

@yyx990803 yyx990803 closed this Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants