-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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(reactivity): ensure that shallow and normal proxies are tracked seperately (close #2843) #2851
fix(reactivity): ensure that shallow and normal proxies are tracked seperately (close #2843) #2851
Conversation
Size report
|
Ah dang, it seems I had the sort-imports extension active involuntarily. That's why there a bunch of changes to the import statementsnthat are unnecessary. Let me know wether I should revert those. |
Aside: I haven't checked but I am pretty sure Simply the idea of wrapping one object in different reactive wrappers is asking for trouble. That issue should be resolved by "please, don't do this" and maybe make a PR to throw when someone attempts that in debug mode. The "least" problem in my opinion that this assumes you have the raw target living somewhere + a reactive wrapper alive somewhere else and you attempts to create a 3rd instance that should be a shallow wrapper. That's 3 different instances for the same object, it's recipe for referential identity problem. The best pattern is to only keep only a single reactive proxy alive. The worst problem is that you have no encapsulation and totally unpredictable code. If I create a If your answer to this is: "no because my code is well organized and will never write back an object that was initially assigned by the shallow proxy"; then organize your code even better and split that object into 2 parts: one clearly deep, the other clearly shallow. Mix and matching based on instances won't lead to nice code. Just the idea of creating proxies all over the place is wacky to me. Wrap your objects at creation time and your life will be much simpler. Happy 2021 everyone! |
reactive proxies and readonly proxies are tracked in individual WeakMaps, so they don't influence each other. And that makes total sense of course, as you often might want to have a reactive proxy internally, i.e. in a composable function, and expose a readonly proxy publicly in its return value.
I kind of agree with that sentiment, at least for The question you raise here is basically wether or not we should allow people to shoot themselves in the foot with these, or force them to find different routes t a solution. Either way, I'd say the current implementation is to be rated a bug as a
This PR does two things:
Whatever we do, we need 1. (or something like it) to actually be able to differentiate bewteen the two. For 2., we could:
However I'm not sure if we actually want to restrict usage in this way? So the question becomes: Are there valid use cases where people should be allowed to tread into this dangerous territory, or should we "protect" people from actually going there even if the purposefully want to? [*]: readonly behaves differently, and I think the change I introduce here to allow both a |
I agree. I would throw in debug mode if someone attempts that. Are you aware of use-cases where creating one shallow and one deep proxy would look like the good solution?
If something appears later on, we can always relax the restriction or come up with a specific design to address the concrete use-case.
Two "something like it" alternative ideas. Single WeakMap might be more efficient but would require being able to detect whether the proxy is shallow or not:
Out of topic:
This is an anecdote about my own usage, but I'm using lots and lots of shallow proxies/refs and the more I do so, the more I feel comforted in that choice. |
i want to connect to you, but fail, because in you pr code, may cause shallowReactive(reactive(shallowReactive(reactive({})))) |
@lijingfaatcumt What behavior would you in that case? |
@LinusBorg in vue3, designing reactiveMap and readonlyMap is for cache the proxy version if the target has the proxy or the target is a proxy, but in you pr, the code below will cause the cache useless let proxy = reactive(origin)
let shallowProxy = shallowProxy(origin)
let proxyCopy = reactive(shallowProxy)
proxyCopy === proxy // false in code above, i want to make proxyCopy === proxy returns true function createReactiveObject(
target: Target,
isReadonly: boolean,
baseHandlers: ProxyHandler<any>,
collectionHandlers: ProxyHandler<any>,
proxyMap: WeakMap<Target, any>
) {
if (!isObject(target)) {
if (__DEV__) {
console.warn(`value cannot be made reactive: ${String(target)}`)
}
return target
}
// target already has corresponding Proxy
const existingProxy = proxyMap.get(target)
if (existingProxy) {
return existingProxy
}
if (proxyMap.has(target)) {
return target
}
// target is already a Proxy, return it.
// exception: calling readonly() on a reactive object
if (
target[ReactiveFlags.RAW] &&
!(isReadonly && target[ReactiveFlags.IS_REACTIVE])
) {
//avoid duplication proxy
target = toRaw(target)
const existingProxy = proxyMap.get(target)
if (existingProxy) {
return existingProxy
}
}
// only a whitelist of value types can be observed.
const targetType = getTargetType(target)
if (targetType === TargetType.INVALID) {
return target
}
const proxy = new Proxy(
target,
targetType === TargetType.COLLECTION ? collectionHandlers : baseHandlers
)
proxyMap.set(target, proxy)
return proxy
} and i pull a pr to you |
Converted this back to a draft as there's both ongoing discussion about the approach itself by @jods4 and an edge case raised by @lijingfaatcumt that isn't covered yet. |
@LinusBorg i have find the new way to solve the problem by #3052 , it use 'isReadonly' and 'isShallow' property to avoid duplication proxy. the createReactiveObject function may modify follow below function createReactiveObject(
target: Target,
isReadonly: boolean,
shallow: boolean,
baseHandlers: ProxyHandler<any>,
collectionHandlers: ProxyHandler<any>
) {
if (!isObject(target)) {
if (__DEV__) {
console.warn(`value cannot be made reactive: ${String(target)}`)
}
return target
}
const proxyMap = getProxyMap(isReadonly, shallow)
// target already has corresponding Proxy
const existingProxy = proxyMap.get(target)
if (existingProxy) {
return existingProxy
}
// target is already a Proxy, return it.
// exception: calling readonly() on a reactive object
if (target[ReactiveFlags.RAW]) {
if (
target[ReactiveFlags.IS_READONLY] === isReadonly &&
target[ReactiveFlags.IS_SHALLOW] === shallow
) {
return target
}
if (!isReadonly || !target[ReactiveFlags.IS_REACTIVE]) {
target = toRaw(target)
const existingProxy = proxyMap.get(target)
if (existingProxy) {
return existingProxy
}
}
}
// only a whitelist of value types can be observed.
const targetType = getTargetType(target)
if (targetType === TargetType.INVALID) {
return target
}
const proxy = new Proxy(
target,
targetType === TargetType.COLLECTION ? collectionHandlers : baseHandlers
)
proxyMap.set(target, proxy)
return proxy
} |
@jods4 i have solve duplication proxy by #3052, for the same origin object, reactive and shallowReactive is different proxy object, i think that is true, because reactive object and shallowReactive object has different baseHandlers |
@lijingfaatcumt Not having 2 distinct instances for the same object is good, but I still have concerns with the concept of having both a deep and a shallow versions of the same object graph and all the unexpected behaviours it leads to. It will unwillingly lead to having both raw and reactive versions of instances deeper in the graph. |
@jods4 in reactivity package, we call function reactive ,but may get a shallowReactive version or readonly version, this will make users confused. in my implements, the problems above will be solved. and this will make related reactive function more specific as the function's name show in addition, the reactive and shallowReactive version have different operating types to the raw target, so i do not think this will be a problem if the reactive and shallowReactive will be stored different places |
@lijingfaatcumt If some code hands you out a readonly object, you should not get around it and start modifying it. If you do so you break the contract and most probably introduce bugs in the code that handed out the readonly object to you. Take Now the next part is more arguable but to me it's the same thing with shallow and deep. If some part of my code created a shallow reactive object graph, it had a reason. If another part of my code takes the same object graph but then makes it deep, it's likely to have unintended consequences. And in a nutshell that's why I think this PR is a bad idea. |
@jods4 first, i think readonly, reactive, shallowReactive only give you different types to operate the orgin objec. they are different proxy object. second we can pass arount the readonly by function toRaw and reactive and then operate the origin object, so the problem you said i pass around the readonly is has existing. this is not my fault. third, call reactive function may getting a readonly confused me. i think function's function should obviously to the function's name is important. in addition, reactive package is standalone, it's design should not affected to much by other packages in vue |
That's explicitly doing hacky stuff. If you really need / want to, it's fine. The problem is that If a library hands out a readonly object to you, then you put it inside an object graph of your own, at no point should you get back a writable version.
This might be because you misunderstood the contract of the function. |
@jods4 i undestand you, and i am wrong, i can not consider the nested proxy |
@LinusBorg @jods4 i find a new way to solve your problems, and i pull a request to @LinusBorg |
694cdc0
to
4b79bac
Compare
This fixes #2843
We currently track reactive proxies in one WeakMap, not differentiating between normal and shallow Proxies. the same is true for readonly proxies.
This means that for each target, only one kind of proxy can be created, because we use these WeakMaps to re-use existing proxies for each known target.
The above test fails -
shallowProxy
will in fact be the same proxy asnormalProxy
.This PR adds separate WeakMaps to track shallow Proxies on their own, which makes the above test pass.