Closed pdanpdan closed 3 weeks ago
I'm :+1: for using shallowRef()
instead of reactive()
for pageContext
as well.
But it means that users will have to deconstruct the wrapper using .value
. Is that an acceptable DX? I'm not that familiar with what Vue users expect.
I guess ref()
is quite ubiquitous and I guess users are very well used to deconstruct with .value
, right?
I'm a little afraid about changing pageContext
usage.
Considering that the shape (object) does not change and that all properties that might be leftovers should be enumerable I would go with a simple cleanup like this: https://play.vuejs.org/#eNp9krFu4zAMhl+F0NIEMFwUd5ORFLgrMtwNl6LXreqgyrSjxJYEiU5TGH730nbdejA6ivwpfvzJVvzyPj03KDKxiToYTxCRGn8rram9CwQtBFSazBmhgyK4Gq5YfyWttNrZSOBViXfOEl7oYVJuP4tWLagMbhJ4yeDpOQGdQdslkGfQ2BwLYzGHbr3wF/8xlX7V3EDXN96/HFFTesK3uFpov04LF3ZKH1ar0xq2t9BKC2AKmIvTg4r7V3sfnMdAb4Nyu4VCVRHXYwVAjhUSLo34dHruJYzT038AqRhNaZeQkvkfXLC5Ht1mn/lBWPtKEfILoG0XLe241eZ6phSJoMi2FaZMj9FZXuFALYV2tTcVhr0nw7ZKwf6N80ihqsq9/h1iFBpMprg+oD4txI/x0sekuA8YMZxRis8cqVAijend/39MO0vWLm8qVn+TfMDoqqZnHGW/+SQYe6YbaP8Mh2hs+Rh3F0Ibp6F60GEJg14Kvsu7b0b/wv2R/pyWJ7p3TmoGfQ==
If you have an idea for a more optimized cleanup it would be nice.
Maybe like this, to reduce the number of updates on pageContextReactive
to only 1: https://play.vuejs.org/#eNp9UrFu4zAM/RVCSxPAda+4m9y4wF3R4W64FG23KIPr0I5SWRIkOk1h+N9LO3HiwSigReTj43skG/HbuXhfo0jEIuReOYKAVLt7aVTlrCdowGOWk9ojtFB4W8EV46+kkSa3JhC4rMQHawgP9Dwg03PRrIEsgdsI3hJYrSPIE2jaCDYJ1GaDhTK4gXY+wcUcQ+ml5hbaS+N3/AyMWr7tMKe4+80mtDB3YT3MNBIohne4WKMpaXsH19cK7lP4cTeHRhqAjthqjLUtZ4orAVQBY9Z4m4Xlh3ny1qGnz1nHtlLrOaRpCkWmA56YYOxldYKtuf/ZdodiN/xODrIQVGmmPERjMpa1uDnuirfEH8LK6YyQfwBNM7mQltssbkZIEQkKbLdQZbwL1vAB9LqlyG3llEa/dKR4HFLw9I+OpMi0th//+hj5GqMhnm8xf5+I78Khi0nx5DGg36MU5xxlvkQ6ph9f/rPaUbKym1oz+pvkM/Kq6k7jEfaHJ8uyR7he7d/+jJUpX8PjgdCEwVQntN9Bj5eCr/rhG+sXuT/jX8PuRPsFJwkbYA==
I think if we do this for pageContext
then we might as well do it for data
.
The main reason for using shallowRef()
instead of reactive()
for data
is performance, right?
Now, pageContext
actually contains data
at pageContext.data
and reactive()
is deep. So, from a performance perspective, I don't think it makes sense to use shallowRef()
for data
while using reactive()
for pageContext
?
For performance we can go with shallowReactive
for pageContext
.
But ignoring performance:
data
can change shape (object<->array) and this cannot be fixeddata
and pageContext
can end up with leftover propertiesFor pageContext
this can be fixed with cleanup (and performance boost by using shallowReactive
) keeping DX.
For data
this can only be fixed by replacing the object (and the only solution I can think of is using shallowRef
and breaking the DX in JS/TS but keeping it in templates.
I didn't know about shallowReactive()
... very neat. Maybe that's what we want for both data
and pageContext
then :thinking:
data
can change shape (object<->array) and this cannot be fixed
IMO this isn't a hard requirement. I think it's ok to tell users:
Make sure
data
is an object. If you want to return a list then do this:- return productList + return { productList }
If we force data to be objects than cleanup + shallow reactive would solve the problems and keep DX (except the DX change induced by forcing data to be non-array object)
On my side, I'm :+1: for using shallowReactive()
+ cleanup for both pageContext
and data
. Also nice because we preserve the current DX (which already forces data to be an object anyways).
We can later see if we want to replace shallowReactive()
with shallowRef()
, depending on whether some user complains about our shallowReactive()
solution. (Because IMO using shallowReactive()
is a hack and the cleanest would be shallowRef()
.)
Different DX:
const a = shallowRef({ y: 1 })
a.value = { x: [ 2 ] } // reactive, you can change the whole content with whatever you want
a.value.x = [ 3 ] // not reactive
a.value.x.push(4) // not reactive
const b = shallowReactive({ x: 1 })
a.x = [ 2 ] // reactive, you cannot change the base (container) object
a.x.push(4) // not reactive
Yea. To me the crux of it is that users don't have to use .value
outside of <template>
if we go for shallowReactive()
.
If you have an idea for a more optimized cleanup it would be nice. Maybe like this, to reduce the number of updates on
pageContextReactive
to only 1
Neat idea, although one issue is that the undefined
keys will still be enumerable.
I don't expect objects to contain that many keys so I think we can go for the straightforward solution. If performance is an issue for some users we can then re-consider.
I went ahead and amended your PR, I hope it's ok and let me know if you disagree.
Looks very good to me, thank you
In
createVueApp
inchangePage
the content ofpageContext
is placed inpageContextReactive
usingObject.assign
. That might lead to leftover properties from previouspageContext
if the new one is missing those properties. The shape changing problem fromdataReactive
should not creep here aspageContext
is always object.