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

fix: In development mode, warns if user tries to Vue.set a property t… #8138

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

bigtunacan
Copy link

@bigtunacan bigtunacan commented May 6, 2018

…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)

  • 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:

…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
Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

Could you add one (or multiple tests) test, please

@bigtunacan
Copy link
Author

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

@chrisvfritz
Copy link
Contributor

I'm trying to add a test for this, but struggling a bit.

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?

@bigtunacan
Copy link
Author

@chrisvfritz I placed the test in test/unit/modules/observer/observer.spec.js.

The test fails with the generic message FAILED Observer Cannot enable reactivity on a property that is already defined

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.

@chrisvfritz
Copy link
Contributor

chrisvfritz commented May 8, 2018

@bigtunacan You may have to wrap your expectation with waitForUpdate (like in this test).

@bigtunacan
Copy link
Author

@chrisvfritz I tried that as well, with the same results.

https://codepen.io/bigtunacan/pen/RyxbLa?editors=0010#

@chrisvfritz
Copy link
Contributor

Can you upload the exact code you're trying? It's OK if the test is failing - we'll squash when we merge anyway.

@bigtunacan
Copy link
Author

@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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be then rather than end.

Copy link
Author

Choose a reason for hiding this comment

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

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({
Copy link

Choose a reason for hiding this comment

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

A Vue instance must have a template prop or render fn, your test is throwing an error before getting to the checks

Copy link
Author

Choose a reason for hiding this comment

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

@dhensche Thanks; that was it.

@bigtunacan
Copy link
Author

@posva I believe this should be ready to merge now.

@bigtunacan
Copy link
Author

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

@yyx990803
Copy link
Member

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.

@bigtunacan
Copy link
Author

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.

@chrisvfritz
Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

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