Skip to content
This repository was archived by the owner on Apr 6, 2023. It is now read-only.

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

Closed
wants to merge 6 commits into from

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Sep 9, 2022

πŸ”— Linked issue

resolves nuxt/nuxt#14839

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 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

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

@danielroe danielroe added bug Something isn't working πŸ”¨ p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage labels Sep 9, 2022
@danielroe danielroe requested a review from pi0 September 9, 2022 22:03
@danielroe danielroe self-assigned this Sep 9, 2022
@netlify
Copy link

netlify bot commented Sep 9, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit f614099
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/632b138d86dfbe0008d7c37d

@pi0 pi0 added the pending label Sep 10, 2022
@pi0
Copy link
Member

pi0 commented Sep 10, 2022

(added pending to land after RC.10)

@pi0 pi0 force-pushed the main branch 2 times, most recently from 247e18b to c98e5c7 Compare September 14, 2022 10:41
@pi0 pi0 removed the pending label Sep 14, 2022
@pi0
Copy link
Member

pi0 commented Sep 14, 2022

@danielroe Would you please help to resolve conflicts? Will do a quick test afterwards to merge πŸš€

@danielroe
Copy link
Member Author

Sure πŸ‘ I am also working on resolving nuxt/nuxt#14573, which may touch some of these files too, but I think this is safe to merge and we can iterate.

@mmis1000
Copy link
Contributor

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.

@danielroe
Copy link
Member Author

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

@mmis1000
Copy link
Contributor

@mmis1000 Yes, it is, and nuxt/nuxt#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.

Copy link
Member Author

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

@mmis1000
Copy link
Contributor

mmis1000 commented Sep 15, 2022

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

Copy link
Member Author

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#14573 at the same time.

@mmis1000
Copy link
Contributor

mmis1000@6bb0859

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.

@atinux atinux closed this in #7940 Oct 8, 2022
@danielroe danielroe deleted the fix/share-suspense branch October 25, 2022 06:27
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3.x bug Something isn't working πŸ”¨ p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nuxtApp.isHydrating is always false in page setup script when using async page
3 participants