vue icon indicating copy to clipboard operation
vue copied to clipboard

warn if $set is used on a property that already exist

Open JJPandari opened this issue 6 years ago • 14 comments

Version

2.5.16

Reproduction link

https://codepen.io/JJPandari/pen/gzLVBq?editors=1010

Steps to reproduce

See the codepen snippet. Follow the comment there to change the vm's data and see what happens.

What is expected?

Even if the prop already exists, using set still makes it reactive, thus trigger view update.

What is actually happening?

Using set later doesn't update the view.


Related source code: https://github.com/vuejs/vue/blob/3eb37acf98e2d9737de897ebe7bdb7e9576bcc21/src/core/observer/index.js#L192 I think most users would expect set to make the prop reactive whenever it's used. I initially opened an issue for the api doc because it wasn't clear (for me) about this. But the comment in the source code is.

JJPandari avatar May 04 '18 02:05 JJPandari

You're setting 2 different setCameLate, your last line should be

this.$set(this, 'setCameLate', 'yes');

Please, next time consider using the forum, the Discord server or StackOverflow for questions first. But feel free to come back and open an issue if it turns out to be a bug 🙂

posva avatar May 04 '18 06:05 posva

Sorry I messed the example up. Try read it again. Or you can read the source code of set I linked to above briefly, it should be easy to see what I'm talking about in the title of this issue. I'm asking about a design decision here, "why not (do it the other way)?"

JJPandari avatar May 04 '18 09:05 JJPandari

Im not sure I get your question, but vue cannot detect the assignment, so you need set.

posva avatar May 04 '18 10:05 posva

@posva I think what @JJPandari would like to do is use Vue.set to make a property reactive after it's already been added, like in this example. I don't see a use case for this, but if the property already exists, I think it would be good to provide a development warning to users, so they know they've done something wrong. For example, for:

Vue.set(this.person, 'job', 'Educator')

If this.person.job had previously been created without Vue.set, so that it's non-reactive, then I think a warning like this would be useful:

[Vue warn]: You tried to use Vue.set on the existing, non-reactive property "job". Properties cannot be made reactive after they've already been added to an object. Instead, use Vue.set where you want to initially create that property.

What do you think?

chrisvfritz avatar May 04 '18 19:05 chrisvfritz

seems fine. I also think it's hardly useful but a warning is ok

posva avatar May 04 '18 20:05 posva

@chrisvfritz Would you see this being logged to the browser console, or something that would be in the build process? Just thinking about taking a stab at this.

bigtunacan avatar May 05 '18 13:05 bigtunacan

it's a runtime dev only warning if you want to add it 🙂

posva avatar May 05 '18 14:05 posva

I think there's a valid use case for setting $set on existing properties. In my application code I have a hashMap object that is initially empty, and I call this.$set(hashMap, object.id, object) after getting the object from API. It will be called more than once if I get updated object sent from API again (apiCall().then(object => this.$set(hashMap, object.id, object))). I don't really want to check if the property exist s before using $set since that would be too verbose.

fnlctrl avatar May 16 '18 07:05 fnlctrl

@fnlctrl It's a bit unclear the way this issue was initially worded, but Chris' codepen is a good example of the real issue here. https://codepen.io/chrisvfritz/pen/rvzgBR?editors=1010

JJPandari's real issue was that they created a non-reactive property then later called $set on the non-reactive property expecting that $set would switch the property from non-reactive to reactive.

The pull request I added on this https://github.com/vuejs/vue/pull/8138 is also addressing only that usage of a call to set. In a non-production environment, it will check if you are trying to set a non-reactive property in which case you would get a warning that said property will not be reactive.

bigtunacan avatar May 16 '18 13:05 bigtunacan

I see... though there's another question: Why not just make it work too when there's already a non-reactive property? 😄 I don't see why there should be a limit.

fnlctrl avatar May 16 '18 16:05 fnlctrl

@fnlctrl My thinking was that if someone first creates an unreactive property, then tries to make it reactive later, any reference to that property in between those two events is very likely to create a difficult-to-diagnose bug.

By showing a warning that they should make the property reactive from the start, we encourage a best practice that eliminates the window for bugs to occur.

chrisvfritz avatar May 16 '18 17:05 chrisvfritz

this issue seems close to me, why does it still say "Open" near the title?

image

alecat88 avatar May 24 '21 11:05 alecat88

It's probably fixed but I'm not sure, haven't used vue recently. @posva Would you link the PR to this issue and close this if fixed?

JJPandari avatar May 28 '21 10:05 JJPandari

Does this problem still exist?

DaZuiZui avatar Jan 22 '23 15:01 DaZuiZui