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

Fix usePage() reactivity in Vue 3 adapter #1469

Merged
merged 6 commits into from
Apr 20, 2023
Merged

Conversation

mathieutu
Copy link
Contributor

@mathieutu mathieutu commented Feb 27, 2023

Hi,
Thanks for your great work on this package.

Why

This PR fixes the lack of reactivity of usePage function return in Vue3 adapter.

Indeed, by returning page.value, we're losing the link with the page ref.
When the page value is fully replaced, the props don't change in the component.

You can find a full simplified example here with a comparison before/after.

References:

How

  • Transform the page object to a reactive
  • Set each other page key independently (with Object.assign)
  • Return the reactive object

Ideally (cf vue docs), it would have been better to return refs, as it was previously (the previous behaviour was the recommended one, unfortunately).
However making this would involve a breaking change. To destructure people can use toRefs(page) (see demo)

Result

const page = usePage()

const { props, component } = toRefs(page)

const posts = computed(() => page.props.posts)
const user = computed(() => props.value.user)

@mathieutu mathieutu changed the title [vue3] Replace page ref to reactive. [vue3] Fix usePage reactivity. Feb 27, 2023
@mathieutu mathieutu marked this pull request as draft February 27, 2023 12:28
@Tofandel
Copy link
Contributor

Tofandel commented Feb 27, 2023

You can actually also revert the previous change and wrap the return of usePage in reactive() to achieve the same behavior

It's a neat trick because if you use reactive in vue it will automatically unref nested values (including computed) for you when you access them so you don't need to use props.value anymore

Here is a demo https://stackblitz.com/edit/vue-awmhha?file=src%2FApp.vue,src%2FusePage.js

@mathieutu mathieutu marked this pull request as ready for review February 27, 2023 14:58
@mathieutu
Copy link
Contributor Author

mathieutu commented Feb 27, 2023

Indeed, I didn't think about it, it's a very cool trick to convert a ref object to a reactive (maybe the one I've been looking for for days?!)
.

BTW. I am looking for a way to have direct access to the props as objects of refs (in pages children components).

const { user, posts } = usePageProps()

Any idea how we could achieve that? (I initially pushed a commit that allowed to do that by Object.assign(page.props, args.props) but it doesn't remove the props that is not needed anymore, so it doesn't work).

The issue is that we
I tried many different ways, but I haven't done any vuejs for a couple of years so my mental model is polluted by React's. 😅

EDIT:
I just had to ask to find it myself actually. It seems to work like that

const usePageProps = () => Object.fromEntries(
    Object.keys(page.props).map((key) => [key, computed(() => page.props[key])])
  );

Feel free to tell if you spot an issue, or if there is a better way 🙂

@Tofandel
Copy link
Contributor

Tofandel commented Feb 27, 2023

Hmm you don't really need a usePageProps if you use reactive

const {props} = usePage() will stay reactive because vue makes every value you access another proxy which is itself reactive without needing to use computed

Basically

const usePageProps = () => usePage().props

https://stackblitz.com/edit/vue-mxpiin?file=src%2FApp.vue,src%2FusePage.js

Or did I misunderstand? Do you really want refs out of there? Usually reactive when working with objects is preferred

I learned all those little tricks developing vue 3 libraries, they are not obvious and it would for sure ease everyone's job if it was plastered around the doc lol, even though I'm not a blogger, maybe it's time I write a blog post about all this hidden features and spread the knowledge

@Tofandel
Copy link
Contributor

Tofandel commented Apr 14, 2023

Oh, will you look at that, another useless force push.. And no reply on this but no problem merging his own PRs to make a release... Just as expected TBH. That's just disappointing. 🙁
For me it's the last drop, I'm jumping ship on this and starting a fork as soon as I got some time

Edit: a nice guy suggested I use hybridly which looks very promising, I'll take a look, thanks

@reinink
Copy link
Member

reinink commented Apr 19, 2023

Hey folks, sorry for the force push on master — bad habit of mine while working on my personal projects, always forget it messes up PRs. I've rebased this PR against master to fix this.

@Tofandel I get that your frustrated, but keep in mind that Inertia is a project of mine that I created for myself initially, and only opened sourced for the benefit of others. Sometimes my responses will be fast, but often they will be slow. That's how open source works. Please don't be rude — I don't deserve it.

@mathieutu Thanks so much for taking a crack at fixing this. Really wish I had caught this issue before releasing 1.0, but that's how it goes sometimes. Going to do some testing on this. Is your latest iteration still a breaking change?

@reinink reinink changed the title [vue3] Fix usePage reactivity. Fix usePage() reactivity in Vue 3 adapter Apr 20, 2023
@reinink
Copy link
Member

reinink commented Apr 20, 2023

@mathieutu @Tofandel Hey thanks so much for your help here — I've just tested this out and it works perfect, plus it's a non-breaking change, so that's a huge win 🥳

FYI, I made a few small tweaks around the internal page object properties.

I'm going to get this merged in and released right now.

@reinink reinink merged commit 76221db into inertiajs:master Apr 20, 2023
@reinink
Copy link
Member

reinink commented Apr 20, 2023

Released: https://github.com/inertiajs/inertia/releases/tag/v1.0.4

@Tofandel
Copy link
Contributor

I get that your frustrated, but keep in mind that Inertia is a project of mine that I created for myself initially, and only opened sourced for the benefit of others. Sometimes my responses will be fast, but often they will be slow. That's how open source works. Please don't be rude — I don't deserve it.

Sorry about that outburst, I already made several comments over the course of the year, I posted constructive feedback and proposed my help. None of which got any reply until now. And I'm not the only person sharing this experience. So yes, frustration got the best of me.

Given that this is a well sponsored project and advertised by laravel I would expect more attention to the community and less unilateralism (peer reviewed releases and maintainers). For the past year most releases introduced more bugs than they solved and then it was radio silence on those issues but I still saw new features and versions released and never any comment on the outlined issues and PR's aimed at solving them.

As a best practice a master branch should be protected on an open source project so that nobody can force push to it not even you. Altering the history of the project is not a good look and messing up PR's is even worse for your community which I was a part of.

For me this has only proven I cannot rely on this project and I've already decided on switching away.

It was fun to play with it and it is a great idea, I got the chance to use it on one project and was aware it was still a work in progress and was hoping to see it grow out of it's flaws, but my feeling is that it went the other way and I just cannot justify using this project in a professional setting for all the given reasons. I bet and it didn't play out

I do wish you all the best with it and hope you'll consider the feedback for a bright future on your project, cheers🙏❤️

@fragkp
Copy link

fragkp commented Apr 20, 2023

@reinink Shouldn't the project be supported by the laravel team since version 1.0 or did I misunderstand something? I remember Taylor had said something about that once. That would be very good for the project.

@emargareten
Copy link

Seems like there was some breaking change with this PR, I just upgraded to v1.0.5 and I get caught TypeError: Cannot read properties of null (reading 'props').
It failed at this code const auth = computed(() => usePage()?.props?.auth) inside a plugin

@onlime
Copy link
Contributor

onlime commented Apr 20, 2023

Seems like there was some breaking change with this PR, I just upgraded to v1.0.5 and I get caught TypeError: Cannot read properties of null (reading 'props').
It failed at this code const auth = computed(() => usePage()?.props?.auth) inside a plugin

Same here. It already broke in v1.0.4

@reinink
Copy link
Member

reinink commented Apr 20, 2023

@emargareten @onlime Hmm, shoot — tested it last night in a TypeScript project, and didn't run into this issue — and I tested it using the same code that you're showing.

Are you able to provide me with a minimal reproduction of this? 🙏

@onlime
Copy link
Contributor

onlime commented Apr 20, 2023

Are you able to provide me with a minimal reproduction of this? 🙏

Minimal reproduction is to install Momentum Modal (you know that project 😉) as recommended in a Jetstream bootstrapped VILT (latest Vue 3, latest Vite) app. Seems to occur here:
https://github.com/lepikhinb/momentum-modal-plugin/blob/d26566f77faeb46ca40ad24912414dd946595d90/src/use-modal.ts#L7

import { usePage } from "@inertiajs/vue3"

const modal = computed(() => usePage()?.props?.modal)

Can't test it right now, but tested this morning in my project with the following skimmed-down app.js:

import './bootstrap'
import '../css/app.css'

import { createApp, h } from 'vue'
import { createInertiaApp } from '@inertiajs/vue3'
import { modal } from 'momentum-modal'
import { resolvePageComponent } from 'laravel-vite-plugin/inertia-helpers'
import { ZiggyVue } from '../../vendor/tightenco/ziggy/dist/vue.m'

const appName = window.document.getElementsByTagName('title')[0]?.innerText || 'Laravel'

createInertiaApp({
    title: (title) => [title, appName].filter(Boolean).join(' - '),
    resolve: (name) => resolvePageComponent(`./Pages/${name}.vue`, import.meta.glob('./Pages/**/*.vue')),
    setup({ el, App, props, plugin }) {
        return createApp({ render: () => h(App, props) })
            .use(modal, {
                resolve: (name) => resolvePageComponent(`./Pages/${name}.vue`, import.meta.glob('./Pages/**/*.vue')),
            })
            .use(plugin)
            .use(ZiggyVue, Ziggy)
            .mount(el)
    },
})

The problem occurs everywhere when using usePage().props.*, e.g. also like I am watching usePage()?.props?.flash.toasts for new toast notifications as described in my blog post:

https://pipo.blog/articles/20230404-vue3-toastify-inertia#toastnotifications-vue-component

@onlime
Copy link
Contributor

onlime commented Apr 20, 2023

follow-up: my FF console reports it breaks in @inertiajs/vue3/src/app.ts line 131 (v1.0.5):

Uncaught TypeError: d.value is null
    props app.ts:131
    run reactivity.esm-bundler.js:190
    get value reactivity.esm-bundler.js:1171
    get2 reactivity.esm-bundler.js:504
    t momentum-modal.js:15
    run reactivity.esm-bundler.js:190
    get value reactivity.esm-bundler.js:1171
    w momentum-modal.js:44
    <anonymous> momentum-modal.js:51
export function usePage<SharedProps extends PageProps>(): Page<SharedProps> {
  return reactive({
    props: computed(() => page.value.props), // <--- line 131
    url: computed(() => page.value.url),
    component: computed(() => page.value.component),
    version: computed(() => page.value.version),
    scrollRegions: computed(() => page.value.scrollRegions),
    rememberedState: computed(() => page.value.rememberedState),
  })
}

UPDATE: @reinink The bug must have been introduced here: 76221db in packages/vue3/src/app.ts:130-137.

and honestly, I never quite understood why I had to start using the optional chaining operatorusePage()?.* at various spots since Inertia v1.0.1 (and as you see in above example, @lepikhinb does the same in Momentum Modal) - so the bug must have already been introduced in that version originally (not in v1.0.0 which was just running fine). See my comment here:

    // NOTE: Since Inertia.js 1.0.1, usePage() may return null initially.
    () => usePage()?.props?.flash.toasts,

@reinink
Copy link
Member

reinink commented Apr 20, 2023

Note for myself later when I take a closer look at this — I wonder if this is the solution:

export function usePage<SharedProps extends PageProps>(): Page<SharedProps> {
   return reactive({
-    props: computed(() => page.value.props),
-    url: computed(() => page.value.url),
-    component: computed(() => page.value.component),
-    version: computed(() => page.value.version),
-    scrollRegions: computed(() => page.value.scrollRegions),
-    rememberedState: computed(() => page.value.rememberedState),
+    props: computed(() => page.value?.props)
+    url: computed(() => page.value?.url),
+    component: computed(() => page.value?.component),
+    version: computed(() => page.value.version),
+    scrollRegions: computed(() => page.value?.scrollRegions),
+    rememberedState: computed(() => page.value?.rememberedState),
   })
 }

@onlime
Copy link
Contributor

onlime commented Apr 20, 2023

Note for myself later when I take a closer look at this — I wonder if this is the solution:

pretty sure yes. But please investigate what has changed from v1.0.0 -> v1.0.1 as the problem (usePage() initially returning null) exists since then, see my updated comment above.

Thanks for looking into it!

@onlime
Copy link
Contributor

onlime commented Apr 20, 2023

oh, maybe v1.0.0...v1.0.1

- const page = ref<Partial<Page>>({})
+ const page = ref<Page<any>>(null)

Why suddenly initializing it with null instead of {}? I'm not really a frontend guy and not into TS, so can't help you any further on this. Hope you'll figure it out.

@onlime
Copy link
Contributor

onlime commented Apr 20, 2023

tested #1530 with a local build in my project and all works as expected again. This also fixes the problem since v1.0.1, so now I can use usePage().props again instead of usePage()?.props.
@reinink Thanks for your efforts!

@DJafari
Copy link

DJafari commented Apr 20, 2023

@reinink In order for you to find the bug better, I will give an explanation that I just realized

if we use usePage composable in external npm package, result of page.value is null! (why ? i don't know )

I had no problem before in version 1.0.2

@reinink
Copy link
Member

reinink commented Apr 20, 2023

@DJafari Hey thanks for that info — what 3rd party library are you using that uses usePage()?

@DJafari
Copy link

DJafari commented Apr 20, 2023

@reinink you're welcome

for mine this is a private package for my custom components for my inertia projects

but you can test with momentum-modal and etc

@reinink
Copy link
Member

reinink commented Apr 21, 2023

Pretty sure I know what’s going wrong here. Looking at the Momentum Modal library, it's using the usePage() hook before Inertia is even initialized — meaning the internal page ref is still null, and so we get this error. I think the solution I suggested earlier (#1469 (comment)) will fix this.

@reinink
Copy link
Member

reinink commented Apr 21, 2023

Fixed in #1530, and released as v1.0.6.

@DJafari
Copy link

DJafari commented Apr 21, 2023

@reinink That error was removed for me, but there is a problem

in 3rd library, page.value always is null and never filled!

i defined this composable in my package :

export default function useBackendUser() {
  const page = usePage()
  return computed(() => page?.props?.auth?.user)
}

and in my inertia app :

const user = useBackendUser()
console.log(user.value) // undefined

but when i use this in my inertia app :

const page = usePage()
const user = computed(() => page?.props?.auth?.user)
console.log(user.value) // my user is here

when i force version to 1.0.3 every things works!

"@inertiajs/vue3": "1.0.3",

@Tofandel

This comment was marked as outdated.

@reinink
Copy link
Member

reinink commented Apr 27, 2023

@DJafari @Tofandel Hey I'm still not totally convinced that our current implementation is wrong. The usePage() hook now always returns a reactive object. Yes, the values within it are nullable, but the returned page is always a reactive object and is never null.

The page object that's referenced in the usePage hook is a ref — meaning it can have an initial value of null and still be reactive — that's how refs work in Vue 3 (as far as I'm aware).

I've tested this new implementation in the Vue 3 adapter with the Momentum Modal library, and it works exactly as expected, so I'm wondering if there is something else going on here with your particular implementation @DJafari. Are you able to please provide a minimal reproduction of this problem as a Git repo for me to test with?

@Tofandel
Copy link
Contributor

I take that back, @reinink is right

export function usePage<SharedProps extends PageProps>(): Page<SharedProps> {
return reactive({
props: computed(() => page.value?.props),
url: computed(() => page.value?.url),
component: computed(() => page.value?.component),
version: computed(() => page.value?.version),
scrollRegions: computed(() => page.value?.scrollRegions),
rememberedState: computed(() => page.value?.rememberedState),
})

usePage never returns null

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

Successfully merging this pull request may close these issues.

vue3 usePage state not updating on props changing
7 participants