Possible leftover properties in `pageContextReactive` after `pageChange` about vike-vue

[brillout]

I'm 👍 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?

[pdanpdan]

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==

[brillout]

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?

[pdanpdan]

For performance we can go with shallowReactive for pageContext.
But ignoring performance:

• data can change shape (object<->array) and this cannot be fixed
• both data and pageContext can end up with leftover properties

For 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.

[brillout]

I didn't know about shallowReactive()... very neat. Maybe that's what we want for both data and pageContext then 🤔

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

[pdanpdan]

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)

[brillout]

On my side, I'm 👍 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().)

[pdanpdan]

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

[brillout]

Yea. To me the crux of it is that users don't have to use .value outside of <template> if we go for shallowReactive().

[brillout]

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.

[pdanpdan]

Looks very good to me, thank you