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

feat(v-model): warn for undefined binding values (#5932) #6734

Closed
wants to merge 1 commit into from

Conversation

chriscasola
Copy link
Contributor

Warn when the v-model binding value is undefined so users are aware that the binding may not be
reactive.

Fixes #5932

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:

I'm not sure if checking if the value is undefined is the right thing to do. I was hoping to do some kind of hasOwnProperty-like check to see if the key in the object exists, but I couldn't figure out how to do that. If that's what makes more sense a pointer in the right direction would be awesome.

@yyx990803
Copy link
Member

Thanks for the PR - unfortunately, this will also warn even if the value is explicitly declared as undefined:

data () {
  return {
    foo: undefined
  }
}

@chriscasola
Copy link
Contributor Author

Yeah...I noticed that but was unsure how to check properly:

I'm not sure if checking if the value is undefined is the right thing to do. I was hoping to do some kind of hasOwnProperty-like check to see if the key in the object exists, but I couldn't figure out how to do that. If that's what makes more sense a pointer in the right direction would be awesome.

Any suggestions on how to check for the property existing?

@posva
Copy link
Member

posva commented Oct 3, 2017

Shouldn't the in operator be enough? if (key in object)

@chriscasola
Copy link
Contributor Author

The in operator isn't enough, because at this point in the code, I think the value of the binding has already been evaluated. So the binding only has two properties, expression and value. And if the key doesn't exist in the data of the component then binding.value is present but the value is undefined.

I think the check needs to be further up the stack but I'm not sure where. I'll keep digging.

Warn when v-model is bound to a non-existant key since that key
will not be reactive.

Fixes vuejs#5932
@chriscasola
Copy link
Contributor Author

@posva @yyx990803 I think I have a solution that works correctly. I'm not sure it's the best way to go about this, but I did a lot of digging and I couldn't figure out any other way to be able to check properties in the second-to-last object in the binding expression.

@yyx990803
Copy link
Member

@chriscasola thanks a lot for looking into this - I just made a comment here: #5932 (comment)

I also implemented a proper warning check strategy in 0fbad1e - but we will probably not use that. Feel free to compare that to your implementation - sorry for your wasted effort, hopefully this can at least help you understand the codebase better and make it easy for further contributions! 🙇

@yyx990803 yyx990803 closed this Oct 4, 2017
@chriscasola
Copy link
Contributor Author

@yyx990803 no problem - thanks for your commit, that definitely clarifies things.

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.

Warn when v-model is bound on non-existent key
3 participants