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

feat: delayed hydration #26468

Draft
wants to merge 235 commits into
base: main
Choose a base branch
from
Draft

Conversation

GalacticHypernova
Copy link
Contributor

@GalacticHypernova GalacticHypernova commented Mar 24, 2024

πŸ”— Linked issue

#24242

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 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)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Lazy loading is a very beneficial addition to the nuxt core. Natively supporting delayed hydration will provide significant performance improvement. Moreover, it will reduce the amount of boilerplate code needed for manual implementations, and will reduce bundle size by not requiring external modules for something that may very well be handled out-of-the-box, as part of the Lazy components behavior.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

TODO:

  • Avoid breaking dynamic imports for components with names beginning with modifiers
  • Infer/retrieve the dimensions of the original component or provide a sensible default to allow for an empty div with the inferred dimensions, thus reducing the SSR payload while still preventing CLS (optional)
  • Add more types of delayed hydration (not necessary, as other types are likely highly personal and depend on per-developer implementation) (optional)

Copy link

stackblitz bot commented Mar 24, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@@ -119,6 +119,79 @@ const show = ref(false)
</template>
```

## Delayed Hydration

In real world applications, some pages may include a lot of content and a lot of components, and most of the time not all of them need to be interactive as soon as the page is loaded. Having them all load eagerly can negatively impact performance and increase bundle size.
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be useful to outline the difference with Lazy prefixed components as it might seem like Lazy should solve this already.

Some rough prose, feel free to word however you like

While Lazy prefixed components are useful for controlling the chunk size of your app, they won't necessarily improve the loading performance. This is because the async component is loaded eagerly with the rest of the page's scripts.

Delayed hydration components fix this, allowing you to load the component just in time to maintain a good user experience while providing the performance benefit of not loading the component immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rephrased it, let me know what you think!

```vue [pages/index.vue]
<template>
<div>
<LazyIdleMyComponent :loader="createIdleLoader({timeout: 3000})"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The loader prop only makes sense if we're passing in a promise, which would only make sense for a generic component.

<LazyMyComponent :loader="createIdleLoader({timeout: 3000})"/>

If you prefer not to create a low-level implementation, that's okay, but I think it's probably better to use a different prop name and the function wouldn't be required.

Copy link
Contributor Author

@GalacticHypernova GalacticHypernova Jun 20, 2024

Choose a reason for hiding this comment

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

I was thinking about it, but since there are defaults for all the components the loader prop is there to allow overrides. Should I name it differently? The function is already not required, as the loader prop is typed, so they can just place the object directly

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes renaming, keep in mind that loader is a prop for defineAsyncComponent in Vue which is a promise, we want to remove ambiguity around what the prop does

Copy link
Contributor

@harlan-zw harlan-zw Jun 20, 2024

Choose a reason for hiding this comment

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

I suggested using loader initially with the thinking that the user would be providing a promise so the signature would match

Copy link
Contributor Author

@GalacticHypernova GalacticHypernova Jun 20, 2024

Choose a reason for hiding this comment

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

Ah, I see, yea that makes sense, should it be a uniform name for all or a different prop for each?
Perhaps hydrate, trigger, config or delayedLoader?

I don't think creating a low-level implementation would provide any substantial benefit if I'm being honest, I don't see where that can be useful. This abstracts away all the boilerplate from the developer for the exchange of a simple prefix.
Do you think a low-level implementation will perform better? I think what counts for performance is the hydration logic, but you're the expert, so I'd love to hear your thoughts on it

/**
* A `requestIdleCallback` options utility, used to determine custom timeout for idle-callback based delayed hydration.
* @param opts the options object, containing the wanted timeout
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

As previous comment, this only makes sense if they return a promise.

Copy link
Contributor Author

@GalacticHypernova GalacticHypernova Jun 20, 2024

Choose a reason for hiding this comment

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

I don't think I understand why it needs to be a promise? Why can it not be a synchronous override? It's just used to override the default settings in here:
https://github.com/GalacticHypernova/nuxt/blob/patch-21/packages/nuxt/src/components/runtime/client-delayed-component.ts#L79

Copy link
Contributor

Choose a reason for hiding this comment

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

from a naming point of view* i'd go for something with a define prefix if it's just for the types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to hydrate

@GalacticHypernova
Copy link
Contributor Author

GalacticHypernova commented Jun 26, 2024

I will add another type of delayed hydration for general purpose conditionals. For never-hydrated components I don't quite know if it's worth making a wrapper over just using a .server component, but I would love to hear your opinion!

Tests and documentations will arrive shortly (marking as draft to take care of them)

@GalacticHypernova
Copy link
Contributor Author

Looks like there was some regression, the tests in dev fail now despite passing previously

}

let observer: IntersectionObserver | null = null
const callbacks = new Map<Element, CallbackFn>()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe can we use a WeakMap ?

Copy link
Contributor Author

@GalacticHypernova GalacticHypernova Jun 29, 2024

Choose a reason for hiding this comment

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

I think so, don't see a reason why not, it would just mean we can't check size and can't disconnect it, but a single observer in memory would likely have minimal performance implications compared to the destroying and reinitialization of it

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

Successfully merging this pull request may close these issues.

None yet

4 participants