fix: In development mode, warns if user tries to Vue.set a property t… #8138
Conversation
…hat already exists. In development mode, warns if user tries to Vue.set a property that already exists. Issue reported in vuejs#8129. Codepen demonstrating the issue available at https://codepen.io/chrisvfritz/pen/rvzgBR?editors=1010 fix vuejs#8129
|
@posva I'm trying to add a test for this, but struggling a bit. Here is a codepen of a test that I expect should pass for this. https://codepen.io/bigtunacan/pen/rvGgoB?editors=0010# |
Could you provide more information on where you struggled? For example, are you not sure where the test should go, or did running the tests not work, etc? |
|
@chrisvfritz I placed the test in The test fails with the generic message So then I ran the tests against Chrome with inspector to see what is happening; the warning is getting hit in test the same as in dev, but yet it fails in my test (which I also linked in the codepen above). I could also push my changes onto this pull request, but I didn't necessarily want to do that when I know the test isn't working yet. |
|
@bigtunacan You may have to wrap your expectation with |
|
@chrisvfritz I tried that as well, with the same results. |
|
Can you upload the exact code you're trying? It's OK if the test is failing - we'll squash when we merge anyway. |
…ng set that already have been c fix vuejs#8129
|
@chrisvfritz Ok; I've updated the PR |
| Vue.set(vm.person, 'job', 'Educator') | ||
| waitForUpdate(() => { | ||
| expect(`Cannot enable reactivity on a property that is already defined`).toHaveBeenWarned() | ||
| }).end(done) |
There was a problem hiding this comment.
I think this should be then rather than end.
There was a problem hiding this comment.
I've tried that as well, but the result is the same.
| @@ -316,6 +316,21 @@ describe('Observer', () => { | |||
| }).then(done) | |||
| }) | |||
|
|
|||
| it('Cannot enable reactivity on a property that is already defined', done => { | |||
| const vm = new Vue({ | |||
There was a problem hiding this comment.
A Vue instance must have a template prop or render fn, your test is throwing an error before getting to the checks
There was a problem hiding this comment.
@dhensche Thanks; that was it.
|
@posva I believe this should be ready to merge now. |
|
@posva Just curious since it's been sitting for awhile and shows as "Changes requested". Are you still waiting on anything from me to merge this in? I thought everything was good now? |
|
I think it makes more sense to convert a property to a reactive one if it's not, which should be the intention in most cases anyways. |
I think the issue is that someone may add a property with standard assignment after the fact as shown in this codepen from the original issue https://codepen.io/chrisvfritz/pen/rvzgBR?editors=1010 Most likely this is not what the person meant to do so it would be good to provide a warning in dev environments. It could be changed to set the property to reactive, but that would seem inconsistent as you would being with a property that is not reactive and then at some point in time it becomes reactive. |
|
I agree with you @bigtunacan. We just had an internal discussion about this in the Vue team and never came up with any valid use cases for making an unreactive property reactive - and the ability just opens a subtle hole for reactivity bugs. So a warning does seem ideal here. |
…hat already exists.
In development mode, warns if user tries to Vue.set a property that already exists. Issue reported
in #8129. Codepen demonstrating the issue available at
https://codepen.io/chrisvfritz/pen/rvzgBR?editors=1010
fix #8129
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
devbranch for v2.x (or to a previous version branch), not themasterbranchfix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information: