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: use createSSRApp to enable more efficient hydration #177

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maoberlehner
Copy link
Contributor

Description 📖

This pull request enables more efficient hydration by using createSSRApp. See: https://vuejs.org/guide/scaling-up/ssr.html#client-hydration

Background 📜

createSSRApp is optimized for hydration of SSR-rendered HTML. This is more efficient because if the HTML stays the same, only event listeners need to be applied but the DOM is left untouched.

The Fix 🔨

By using createSSRApp we get all the benefits of efficient hydration.

@nx-cloud
Copy link

nx-cloud bot commented Aug 7, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit da262f3. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@maoberlehner
Copy link
Contributor Author

I updated the previously reverted pull request to only replace comments for non html files. As I understand it it should only be necessary in those cases.

@@ -17,7 +15,7 @@ export default function createVueIsland (component: Component, id: string, el: E
appDefinition.name = `Island: ${nameFromFile(component.__file)}`

const app = createVueApp(appDefinition)
app.mount(el!, Boolean(slots))
app.mount(el!, true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite sure what the hydration setting has to do with slots. And as far as i could see Boolean(slots) was always true anyway. In my (few) tests, slots was either {} or {foo:'bar'} which both are truthy.

Copy link
Owner

Choose a reason for hiding this comment

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

Probably outdated code from one of the first versions.

I've removed the second parameter this in this branch since createSSRApp will override mount to always pass true.

return content
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ElMassimo
Copy link
Owner

ElMassimo commented Aug 7, 2022

Hi Markus!

Thanks for taking another look at this.

I'll take a look at this branch during the week, but I think it will suffer from mismatches in some situations, given on how the Vue renderer is used when prerendering.

I have a branch where I've addressed these differences in the hydration comments Vue injects, preventing mismatches, but I haven't merged it because the chosen approach loses HMR on mounted islands, so I need to revisit it.

Perhaps avoiding the more strict createSSRApp during development will give us the right balance, between efficient hydration in production, and no warnings and full HMR during development.

@maoberlehner
Copy link
Contributor Author

As I understand it createApp vs. createSSRApp is not about strictness but about the former not caring at all about existing DOM, rendering everything from scratch and throwing away SSRd HTML. While the later ideally does not render anything but just binds event listeners to the existing DOM.

Not knowing a lot about the internals I don't quite understand why mismatches can't be avoided. Anyway, personally I don't care about dev mode. If it is simpler to do using createdApp during dev, I'm all for it.

But having optimized hydration in production mode seems crucial to me, considering the ultimate goal of this project being max performance by avoiding full hydration.

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.

2 participants