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

Computed value without setter is not writable #1135

Open
LongTengDao opened this issue May 6, 2020 · 4 comments
Open

Computed value without setter is not writable #1135

LongTengDao opened this issue May 6, 2020 · 4 comments

Comments

@LongTengDao
Copy link
Contributor

LongTengDao commented May 6, 2020

What problem does this feature solve?

https://github.com/vuejs/vue-next/blob/d66211849ca174c4458b59d3df5569730ee224f6/packages/reactivity/src/baseHandlers.ts#L95-L96

Computed value without setter (and the default setter is production ? noop : warn) is not writable, here should skip this case, don't set target[key].value, and leave it to the rest part of function to set target[key].

But before PR, I want to ask clearly -- why here set target[key].value preferentially, rather than write target[key]? Why not treat everything as the same, no matter it is ref (readonly or not) or computed (getter only or with setter)?

If we fix this problem, getter-only computed value is skipped, how should we treat the others? I think the rule should be explicit and written in spec doc.

What does the proposed API look like?

@yyx990803
Copy link
Member

It should still attempt the write to trigger the warning. Otherwise it just fails silently.

@LongTengDao
Copy link
Contributor Author

LongTengDao commented May 6, 2020

@yyx990803 Why not set on the target to replace that computed like the other cases do?

https://github.com/vuejs/vue-next/blob/d66211849ca174c4458b59d3df5569730ee224f6/packages/reactivity/src/baseHandlers.ts#L104

While running this function, user is just trying to set value on target[key], not on target[key].value, the latter is just an preferred optimization. A warning and failure just because optimization error which can't be avoid will be amazing.

import { computed, reactive, ref } from 'reactivity';
const a = computed(() => 1);
const data = reactive({ a });
data.a = 2;
// warning and fails -- and can't avoid, unless:
data.a = ref(2);
// Is this intended?
// If so, throw an error would be better than just warn.
// like write to readonly value in strict mode throwing an error.

How do you think?

@LinusBorg
Copy link
Member

The latter doesn't seem intended to me, rather like a bug.

@HcySunYang
Copy link
Member

The latter made a rewrite of Ref, using ref(2) to replace the computed ref a. Details can be found here https://github.com/vuejs/vue-next/blob/master/packages/reactivity/__tests__/reactive.spec.ts#L200-L213. But I think we should ban this behavior, the first reason is that it will cause TS type mismatch:

image

Another reason is that ban this behavior can avoid confusion for users, as @LongTengDao showed above.

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

No branches or pull requests

4 participants