#7981 is a breaking change

[VadymGidulian]

Version

2.5.18-beta.0

Reproduction link

https://jsfiddle.net/50wL7mdz/806315/

Steps to reproduce

Change src attributes in HTML to try w/ different versions

What is expected?

isChanged computed property to be true after delay

What is actually happening?

it doesn't get an updated value of isChanged

---

Let's consider a bit simplified situation of my real one. Suppose I have a DataWrapper which purpose is to tell whether the held data object was changed or not.

This wrapper has 2 properties: isChanged (read-only, boolean, false by default) and data (read-write, any type); and it is initialized w/ some data.

I think it's clear and not a problem that isChanged property should only have a getter, but for the sake of reactivity, it's allowed to have setter be added. Also, in data setter there's a line

try { this.isChanged = true } catch (e) {/*ignore*/}

for this purpose, i.e. if a setter was defined by some reactivity library (built into Vue in our case), we should invoke it before changing the internal state to let it pass the Vue's check and propagate the change across dependent values:

if (newVal === value || (newVal !== newVal && value !== value)) {
    return
}

But then, since we hadn't any setter initially defined for isChanged property, the following check

// #7981: for accessor properties without setter
if (getter && !setter) { return }

, which was added in PR #7981, is failed and the change is not propagated, which is definitely not a desired behavior.

Commenting of this line completely solves the problem.

So I consider this change as a breaking and think it should be reverted to original behavior.

[posva]

It's working fine with latest release (v2.5.21)

[VadymGidulian]

@posva
But the latest version is used in https://jsfiddle.net/50wL7mdz/806315/ and it does not work, isn't it?
isChanged CP did not updated after delay
And you can see that this behavior differs in 2.5.17 if you use it instead of the latest version

Here's the same code, but with the version displayed:
https://jsfiddle.net/50wL7mdz/806414/

[posva]

Oh, you meant the is changed at the top. That one isn't changing.
Well, that was never something similar to what we do in Vue apps. I wouldn't consider this a breaking change as the usage wasn't supported in the first place. Let me share this with others though

[VadymGidulian]

Yes, probably not. But my considerations were the following:

1. refactor: make the set method more reasonable #7981 is classified as a refactoring
2. The behavior of accessors was changed by that PR in all following versions
3. Reverting that PR's change completely solves the problem in all following versions and restores the original behavior

[LinusBorg]

Well, I see it like this: #7981 was introduced to skip unnecessary obervations:

In a usual scenario, an object property that only has a getter and no setter can only be changed by other means unknown to Vue. Vue can only track changes through a setter, which the object in question apparently doesn't use. So there's no use in making this object reactive. 99 times out of a 100, we would otherwise observe an object property that, as far as Vue can tell, won't ever change.

Now, before this PR, we did add this setter, and you are the 1 in a 100 case where the author actively checks for this setter to call it just to accomodate Vue.

But I'm not really willing to call this a breaking change as we never made that implementation detail part of our public API contract. To the contrary, we have warnings like this one in our docs:

The object must be plain: native objects such as browser API objects and prototype properties are ignored. A rule of thumb is that data should just be data - it is not recommended to observe objects with their own stateful behavior.
source

You are of coure free to work around these limitations like you did, but it's not our responsibility to think of each possible interaction with stateful objects in reactive data (that we explicitly warn against) when doing internal changes.

[VadymGidulian]

Hm... Yes, I forgot about this warning in docs... You are right, it wasn't guaranteed to work in my case.

I examined the code more carefully. Indeed, there's no other possibility to comply with my needs w/o reducing performance, which is not the case due to the rarity of the use case.

Thanks for your replies!

[LinusBorg]

Thanks for understanding. I hope you find a good way to solve your issue