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

fix(nuxt): page hydration and double load #7940

Merged
merged 11 commits into from Oct 8, 2022

Conversation

mmis1000
Copy link
Contributor

@mmis1000 mmis1000 commented Oct 1, 2022

πŸ”— Linked issue

Fix nuxt/nuxt#14839, fixes nuxt/nuxt#14573, closes #7400

❓ 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 is a remake of #7400

Besides of fix and workaround in #7400.

  1. This PR use a different approach to handle hydrating, so change of page wrapper isn't required.
  2. This PR ensure that layout and any page wrapper never change outside of the window that route navigation happens.
  3. This PR adds proper test for linked issue
    • so regression after this should be noticed

A remaining issue is temporary blank page when switching between layout and navigate at the same time.
But that isn't fixable without vuejs/core#6736.
So it isn't handled here.

πŸ“ Checklist

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

@codesandbox
Copy link

codesandbox bot commented Oct 1, 2022

CodeSandbox logoCodeSandbox logoΒ  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@netlify
Copy link

netlify bot commented Oct 1, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit bffe838
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/63417ac98b50230009b5f01c

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me - thank you so much 😊

I've made a couple of minor changes and one fix (to ensure this works for layouts that are children of <NuxtPage>); let me know if you spot any issues.

P.S. really appreciate the work on the tests...

@mmis1000
Copy link
Contributor Author

mmis1000 commented Oct 1, 2022

This looks great to me - thank you so much 😊

I've made a couple of minor changes and one fix (to ensure this works for layouts that are children of <NuxtPage>); let me know if you spot any issues.

P.S. really appreciate the work on the tests...

Looks fine to me except I am wondering what is this for 21d872c#diff-ef9ac7f796e2b5d9a4a232b098d7ce5dd92610191d35dfaf10612a76bfe532ddR50

Is this exist to ensure that layout used inside page changes when writing things like useRoute().meta.layout = xxx inside a page component?

Also, this didn't seem to change any test results, is there a test for it?

@danielroe
Copy link
Member

danielroe commented Oct 1, 2022

Yes, so, for example if you are changing page which has a layout within it, it doesn't force rerender the existing page with the new layout before suspense resolves. And yes, we should add a test for that case. I'll do that later unless you get to it first.

@mmis1000
Copy link
Contributor Author

mmis1000 commented Oct 2, 2022

Yes, so, for example if you are changing page which has a layout within it, it doesn't force rerender the existing page with the new layout before suspense resolves. And yes, we should add a test for that case. I'll do that later unless you get to it first.

Yea, I think it would makes more sense in anthoer PR instead of this due to following reason.

  1. That isn't actually part of this change set
  2. You can't test this without alternative app.vue. It need a another fixture of app because the default app.vue defaults to have a layout in it.
    • To test this, the change set would be too big for this PR.

@danielroe
Copy link
Member

I have great sympathy for your desire to push it to another PR, but I think it's not as complex as you are worried it will be.

It's just a matter of creating two files:

~/pages/internal-layout.vue and ~/pages/internal-layout/[slug].vue. We can turn off normal layouts in internal-layout.vue with definePageMeta({ layout: false }) and take full control by adding our custom <NuxtLayout> inline in the page.

I do think it would be good to add tests for this in this PR as it would be good to check that 21d872c is doing what it's meant to do, and that we don't accidentally regress the fix.

@mmis1000
Copy link
Contributor Author

mmis1000 commented Oct 3, 2022

I have great sympathy for your desire to push it to another PR, but I think it's not as complex as you are worried it will be.

It's just a matter of creating two files:

~/pages/internal-layout.vue and ~/pages/internal-layout/[slug].vue. We can turn off normal layouts in internal-layout.vue with definePageMeta({ layout: false }) and take full control by adding our custom <NuxtLayout> inline in the page.

I do think it would be good to add tests for this in this PR as it would be good to check that 21d872c is doing what it's meant to do, and that we don't accidentally regress the fix.

https://stackblitz.com/edit/github-42bhq4?file=layouts%2Ftest3.vue,pages%2Findex.vue

I think it will still also change the outer layout even you already set layout: false. So you need a App.vue without outer layout to test it. (this playground is in current rc version)

@tada5hi
Copy link

tada5hi commented Oct 4, 2022

would love to see this pr merged and released πŸ™ˆ

@danielroe danielroe self-assigned this Oct 4, 2022
@danielroe
Copy link
Member

The only thing it needs is the test I mentioned previously. I'm on it.

@danielroe danielroe added the bug Something isn't working label Oct 4, 2022 — with Volta.net
@tada5hi
Copy link

tada5hi commented Oct 8, 2022

any news on this ?

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@danielroe danielroe assigned Atinux and unassigned danielroe Oct 8, 2022
@Atinux Atinux merged commit c404cb1 into nuxt:main Oct 8, 2022
Copy link
Member

Atinux commented Oct 8, 2022

Thank you very much!

It would be nice to have a way to detect how much kb add pull requests to the Nuxt default bundle size.

@loilo
Copy link

loilo commented Oct 8, 2022

So, just out of curiosity (because I ran into nuxt/nuxt#14573 as well) – when is the next RC scheduled to be available? πŸ₯Ή

@danielroe
Copy link
Member

Until then, you can try this out in the edge channel.

@loilo
Copy link

loilo commented Oct 8, 2022

Thank you, will take a look. πŸ₯³

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
Projects
None yet
5 participants