search

vikejs / vike-vue

🔨 Vue integration for Vike
https://vike.dev/vike-vue
MIT License
33 stars 5 forks source link

fix: cleanup old keys and use shallowReactive instead of reactive for dataReactive and pageContextReactive #116, #121 #120

Closed pdanpdan closed 3 weeks ago

pdanpdan commented 3 weeks ago

BREAKING CHANGE: data from +data hook must be a non-array object (if you need to return a list wrap it in an object) BREAKING CHANGE: both useData and usePageContext return shallowReactive instead of reactive

closes #116, closes #121

pdanpdan commented 3 weeks ago

A fix for pageContextReactive is still needed (in changePage)

brillout commented 3 weeks ago

I think I found a use case where this unfortunately doesn't work. Let me try to reproduce on /examples/full/.

pdanpdan commented 3 weeks ago

BTW, if we want to treat data as readonly we could go with shallowRef I'll try after we check possible breaking case(s)

brillout commented 3 weeks ago

https://github.com/vikejs/vike-vue/commit/984ef6073e61f09a0d8f4598d762815ddc9f5ce2 breaks this PR.

To reproduce:

  1. cd examples/full/
  2. pnpm run dev
  3. localhost:3000/hello

    Observe text: "Hi anonymous."

  4. Click on /hello/eli.

    Observe text is still: Hi anonymous..
    (It should be Hi eli..)

  5. Refresh page.

    Observe text is now: Hi eli..

pdanpdan commented 3 weeks ago

It looks like we need to return the full ref, so the DX changes in JS/TS (stays the same in template)

brillout commented 3 weeks ago

in order to keep reactivity we need to expose the shallowref

Maybe a hack would be:

const dataRef = ref(data)
Object.assign(dataRef, data)

Not sure if it's worth it. I don't know whether data.value.product.id is a weird DX for Vue users.

pdanpdan commented 3 weeks ago

I don't know whether data.value.product.id is a weird DX for Vue users.

Not weird DX, especially if editor interprets TS types you get full autocomplete

Maybe a hack would be: ...

I'm not sure where would that fit

brillout commented 3 weeks ago

Ok :+1:

On my side I think we should go for this shallowRef() solution then.

Thoughts on pageContext? In principle, it should also just be a shallowRef() I think. But what about the DX :thinking:

pdanpdan commented 3 weeks ago

I'll clean up this PR and let's move to another issue for pageContext

brillout commented 3 weeks ago

:+1: And let's also decide about pageContext before merging. Probably best to holistically decide for both.

pdanpdan commented 3 weeks ago

Cleanup done here and opened sibling issue https://github.com/vikejs/vike-vue/issues/121

pdanpdan commented 3 weeks ago

This should be now ready

brillout commented 3 weeks ago

I made some minor changes, let me know if you disagree with something. Otherwise I'll squash, merge, and release.

pdanpdan commented 3 weeks ago

LGTM

brillout commented 3 weeks ago

Merged :tada:

Thank you the neat collaboration :green_heart:

brillout commented 3 weeks ago

And released as 0.7.2 :rocket:

@4350pChris I went ahead without your input because this PR fixes a couple of bugs without introducing any (conceptual) breaking change. But I'm more than happy to have the conversation whether we should go for shallowRef() instead of shallowReactive() (or something entirely else) with you — in case you disagree with some of the discussions we had at https://github.com/vikejs/vike-vue/issues/116 and https://github.com/vikejs/vike-vue/issues/121.