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

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

Open
JJPandari opened this issue May 4, 2018 · 14 comments · May be fixed by #8138
Open

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

JJPandari opened this issue May 4, 2018 · 14 comments · May be fixed by #8138

Comments

@JJPandari
Copy link

JJPandari commented May 4, 2018

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:

* Set a property on an object. Adds the new property and
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.

@posva
Copy link
Member

posva commented May 4, 2018

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 posva closed this as completed May 4, 2018
@JJPandari
Copy link
Author

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)?"

@posva
Copy link
Member

posva commented May 4, 2018

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

@chrisvfritz
Copy link
Contributor

@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?

@posva
Copy link
Member

posva commented May 4, 2018

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

@posva posva reopened this May 4, 2018
@posva posva changed the title when using $set, why not make the prop reactive when is already exists? warn if $set is used on a property that already exist May 4, 2018
@bigtunacan
Copy link

@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.

@posva
Copy link
Member

posva commented May 5, 2018

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

bigtunacan added a commit to bigtunacan/vue that referenced this issue May 6, 2018
…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
bigtunacan added a commit to bigtunacan/vue that referenced this issue May 8, 2018
@fnlctrl
Copy link
Member

fnlctrl commented May 16, 2018

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.

@bigtunacan
Copy link

@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 #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.

@fnlctrl
Copy link
Member

fnlctrl commented May 16, 2018

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.

@chrisvfritz
Copy link
Contributor

@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.

@alecat88
Copy link

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

image

@JJPandari
Copy link
Author

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?

@DaZuiZui
Copy link

Does this problem still exist?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants