framework icon indicating copy to clipboard operation
framework copied to clipboard

fix(nuxt): track suspense status so we can share single state

Open danielroe opened this issue 3 years ago • 11 comments

🔗 Linked issue

resolves #7337

❓ Type of change

  • [ ] 📖 Documentation (updates to the documentation or readme)
  • [x] 🐞 Bug fix (a non-breaking change that fixes an issue)
  • [ ] 👌 Enhancement (improving an existing functionality like performance)
  • [ ] ✨ New feature (a non-breaking change that adds functionality)
  • [ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This PR:

  1. resolves a bug where we generated 'too many' Suspense components, meaning that isHydrating was false in nested child pages on initial load (which has bad consequences for data fetching, as we make decisions based on whether it's the first load or not).
  2. works around a vue core issue where nothing under an async component is correctly wrapped into a parent suspense (so this would include all pages with a <NuxtLayout>) meaning that isHydrating is incorrectly false for these entire trees.

📝 Checklist

  • [x] I have linked an issue or discussion.
  • [ ] I have updated the documentation accordingly.

danielroe avatar Sep 09 '22 22:09 danielroe

Deploy Preview for nuxt3-docs canceled.

Name Link
Latest commit f6140997a610ee929faf6c724e1bcb77531e4115
Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/632b138d86dfbe0008d7c37d

netlify[bot] avatar Sep 09 '22 22:09 netlify[bot]

(added pending to land after RC.10)

pi0 avatar Sep 10 '22 10:09 pi0

@danielroe Would you please help to resolve conflicts? Will do a quick test afterwards to merge 🚀

pi0 avatar Sep 14 '22 17:09 pi0

Sure 👍 I am also working on resolving https://github.com/nuxt/nuxt.js/issues/14573, which may touch some of these files too, but I think this is safe to merge and we can iterate.

danielroe avatar Sep 14 '22 17:09 danielroe

https://github.com/nuxt/framework/pull/7400/files#diff-38b334f71e61b2fb35956472399fb0c87ca4a9d84ef6f99c570c973115f20f0fR49

Isn't change of suspense wrapper one of the reason that page loaded twice?

https://stackblitz.com/edit/github-fktkzl-zxyvww?file=src%2FApp.vue

I don't think vue can keep the page instance if the wrapper changed.

mmis1000 avatar Sep 15 '22 02:09 mmis1000

@mmis1000 Yes, it is, and https://github.com/nuxt/nuxt.js/issues/14573 exists to track it. I don't believe it's affected by this PR; please let me know if you think otherwise.

danielroe avatar Sep 15 '22 08:09 danielroe

@mmis1000 Yes, it is, and nuxt/nuxt.js#14573 exists to track it. I don't believe it's affected by this PR; please let me know if you think otherwise.

I think use of any additional reactive property that does not change at the same time of route in the render function of NuxtPage component is a entirely bad idea. (Just like in this PR, which introduced more reactive state.). Because any structure change of parent of page component WILL RELOAD the whole page.

mmis1000 avatar Sep 15 '22 09:09 mmis1000

This PR deliberately doesn't make it reactive. So it should only change the suspense structure if there is already a rerender happening.

danielroe avatar Sep 15 '22 09:09 danielroe

This PR deliberately doesn't make it reactive. So it should only change the suspense structure if there is already a rerender happening.

https://stackblitz.com/edit/github-gagmal-vjqqo3?file=pages%2Fb%2F[id].vue

That line alone is already enough to cause bug.

  1. Navigate to /b/b1
  2. reload page to get a SSR's initial page
  3. click b2 to go to /b/b2
  4. page is reloaded even it shouldn't (The page key is already specified as b via definePageMeta) a. Notice: There is a new 'Suspense' mark on the nested page component after this navigation.
  5. navigate to b1 again
  6. it works correctly this time, page is not reloaded

mmis1000 avatar Sep 15 '22 10:09 mmis1000

You're right, it does cause an unnecessary rerender. We may pend this PR as I have local work-in-progress that doesn't cause it, and it makes sense to resolve nuxt/nuxt.js#14573 at the same time.

danielroe avatar Sep 15 '22 10:09 danielroe

https://github.com/mmis1000/nuxt-framework/commit/6bb08590805a16d514dff5f63262af9e1acae796

I think one of potential fix would be using a counter to count how many <Suspense> we created, and delay the resolve until every one resolved?

By the way, is the isNested check there to workaround some issue? It seems that stop all redirection between sibling of nested routes (/a/a1 -> /a/a2) to emit page:start and page:finish even there is a page load.

mmis1000 avatar Sep 15 '22 14:09 mmis1000