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

test($compile): #7048 warn if both v-model and v-bind:value used on same element #7056

Merged
merged 4 commits into from
Nov 16, 2017
Merged

test($compile): #7048 warn if both v-model and v-bind:value used on same element #7056

merged 4 commits into from
Nov 16, 2017

Conversation

rpemberton
Copy link

Address issue #7048

I don't have much experience with contributing so any feedback would be very welcome.

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

@rpemberton rpemberton changed the title test($compile): #7048[,#7048] warn if both v-model and v-bind:value used on same text input test($compile): #7048 warn if both v-model and v-bind:value used on same text input Nov 14, 2017
(!map['type'] || map['type'] === 'text')
) {
var vBind = map['v-bind:value'] ? 'v-bind:value' : ':value'
warn('v-model and ' + vBind + ' used on the same text input')
Copy link
Member

Choose a reason for hiding this comment

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

We could definitely use a more detailed message here - something like "v-bind:value conflicts with v-model on the same element because the latter already expands to a value binding internally"

process.env.NODE_ENV !== 'production' &&
map['v-model'] &&
(map['v-bind:value'] || map[':value']) &&
(!map['type'] || map['type'] === 'text')
Copy link
Member

Choose a reason for hiding this comment

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

This warning should apply to anything that is not:

  • type=checkbox
  • type=radio
  • select

@rpemberton
Copy link
Author

Thanks for the feedback @yyx990803

Those changes have now been made. Do you think the tests are sufficient?

@rpemberton rpemberton changed the title test($compile): #7048 warn if both v-model and v-bind:value used on same text input test($compile): #7048 warn if both v-model and v-bind:value used on same element Nov 14, 2017
) {
var vBindValue = map['v-bind:value'] ? 'v-bind:value' : ':value'
warn(vBindValue + ' conflicts with v-model on the same element because the latter already expands to a value binding internally')
}
Copy link
Member

Choose a reason for hiding this comment

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

Check looks good now - but I'm not sure if makeAttrsMap is the best place to do this check. Let's place it here since this warning is about v-model.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, it's now been moved

@yyx990803 yyx990803 merged this pull request into vuejs:dev Nov 16, 2017
@rpemberton rpemberton deleted the model-bind-error branch November 16, 2017 17:14
yyx990803 pushed a commit that referenced this pull request Nov 16, 2017
* test($compile): warn if v-model and :value used on same text input

#7048

* test($compile): make v-model and v-bind:value warning apply to all but exceptions

#7048

* test($compile): move v-model/:value conflict warner to model.js

#7048

* style: split long warning messages onto new lines
lovelope pushed a commit to lovelope/vue that referenced this pull request Feb 1, 2018
…js#7056)

* test($compile): warn if v-model and :value used on same text input

vuejs#7048

* test($compile): make v-model and v-bind:value warning apply to all but exceptions

vuejs#7048

* test($compile): move v-model/:value conflict warner to model.js

vuejs#7048

* style: split long warning messages onto new lines
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
…js#7056)

* test($compile): warn if v-model and :value used on same text input

vuejs#7048

* test($compile): make v-model and v-bind:value warning apply to all but exceptions

vuejs#7048

* test($compile): move v-model/:value conflict warner to model.js

vuejs#7048

* style: split long warning messages onto new lines
aJean pushed a commit to aJean/vue that referenced this pull request Aug 19, 2020
…js#7056)

* test($compile): warn if v-model and :value used on same text input

vuejs#7048

* test($compile): make v-model and v-bind:value warning apply to all but exceptions

vuejs#7048

* test($compile): move v-model/:value conflict warner to model.js

vuejs#7048

* style: split long warning messages onto new lines
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants