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

[Performance] Using this.$route inside a computed triggers reactivity while we are changing pages #27272

Closed
RodrigoProjects opened this issue May 18, 2024 · 10 comments · Fixed by #27313

Comments

@RodrigoProjects
Copy link
Contributor

Environment

  • Operating System: Darwin
  • Node Version: v20.10.0
  • Nuxt Version: 3.11.2
  • CLI Version: 3.11.1
  • Nitro Version: 2.9.6
  • Package Manager: pnpm@8.13.1
  • Builder: -
  • User Config: ssr, experimental, sourcemap, app, hooks, devServer, modules, devtools, imports, vite, nitro, components, pinia, i18n, runtimeConfig
  • Runtime Modules: @internal/cms-renderer@1.0.0, @internal/module-proxy@1.0.0, @nuxtjs/i18n@8.2.0, @nuxt/devtools@1.0.6
  • Build Modules: -

Reproduction

https://stackblitz.com/edit/github-89dmst?file=pages%2F[...slug].vue

Describe the bug

Issue:

  • When using options API with Vue and creating a computed that utilizes this.$route inside it, the computed will trigger when we click on a NuxtLink while we are still changing pages. Being inside a computed seems special because if you inject $route directly into the template it does not update until page change.

Observing the issue should be easy with the provided reproduction.

Notes:

  • The same cannot be said when we use Composition API and the useRoute composable, if we create a computed that uses the value of the composable inside it will not trigger during a route change (correct behavior for me).
  • The first query object is a direct template injection of $route, the second one is a computed using this.$route inside and you can see that it updates while changing pages, the third one is a computed using the useRoute composable value and it only updates after route change.

Impact:

  • Currently our big E-Commerce website has some heavy computations associated with the this.$route while we are migrating to full Composition API, each page change it's triggering some computeds and, by consequence, patching a full component with a lot of data.
  • I got a workaround by having a proxy route computed that checks if the route the computed is re-calculating it's the same route the component was originally created at (not scalable for our use case).

Additional context

No response

Logs

No response

Copy link

stackblitz bot commented May 18, 2024

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

@danielroe
Copy link
Member

The issue comes from vue-router and suspense - we need to ensure that within each suspense fork we have different route objects. We in fact even do custom handling within <template> (injecting a custom route object) and for useRoute...

We probably need to port this fix to this.$route as well, if possible.

@jakubednar
Copy link
Contributor

Hello 👋, I started working on this issue, but I got a little stuck. I didn't manage to make it work, but from investigation it looks like this.$route inside computed is not waiting for page:finish hook and it gets the new route object right away.

@danielroe
Copy link
Member

@jakubednar This might help. It's how we ensure that we use the provided route rather than this.$route:

if (!INJECTION_SINGLE_RE.test(code) || code.includes('_ctx._.provides[__nuxt_route_symbol')) { return }
let replaced = false
const s = new MagicString(code)
s.replace(INJECTION_RE, () => {
replaced = true
return '(_ctx._.provides[__nuxt_route_symbol] || _ctx.$route)'
})
if (replaced) {
s.prepend('import { PageRouteSymbol as __nuxt_route_symbol } from \'#app/components/injections\';\n')
}

@jakubednar
Copy link
Contributor

Just found out that this.$route inside computed is getting triggered twice. Added console.log there while debugging.
2024-05-21 10 17 13

@RodrigoProjects
Copy link
Contributor Author

RodrigoProjects commented May 22, 2024

I could create a Vue plugin and overwrite the $route with the Nuxt provided route. What you think of this as potential fix? @danielroe

Doing a replace like you did will be hard for this case when using options API

@jakubednar
Copy link
Contributor

jakubednar commented May 22, 2024

I tried fixing it yesterday, but all I managed to affect was useRoute, $route inside template. Maybe I'm doing something wrong, but I couldn't affect this.$route inside script.
I will be grateful for any help with this.

@RodrigoProjects
Copy link
Contributor Author

RodrigoProjects commented May 22, 2024

I tried fixing it yesterday, but all I managed to affect was useRoute, $route inside template. Maybe I'm doing something wrong, but I couldn't affect this.$route inside script. I will be grateful for any help with this.

After further investigation, I think you can do something like this:
`js
const INJECTION_RE = /\bthis.$route\b/g
if (!INJECTION_SINGLE_RE.test(code) || code.includes('ctx..provides[__nuxt_route_symbol')) { return }

let replaced = false
const s = new MagicString(code)
s.replace(INJECTION_RE, () => {
replaced = true
return '(this._.provides[__nuxt_route_symbol] || this.$route)'
})
if (replaced) {
s.prepend('import { PageRouteSymbol as __nuxt_route_symbol } from '#app/components/injections';\n')
} `

Did this very fast but I think you get the general idea, do a replace for this.$route too and inject the "nuxt route" and fallback to this.$route like daniel did for the _ctx case!

@jakubednar
Copy link
Contributor

I did something similar, but i got error thatthis is undefined. But I did it with dynamic regex which supported both ctx and this cases. I'm going to try to handle just this.$route case.

@RodrigoProjects
Copy link
Contributor Author

Thanks for making the PR for me @jakubednar time saver! <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants