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

poc: ssr client rehydration #2442

Merged
merged 12 commits into from
Oct 26, 2021
Merged

poc: ssr client rehydration #2442

merged 12 commits into from
Oct 26, 2021

Conversation

jodarove
Copy link
Contributor

@jodarove jodarove commented Jul 30, 2021

Details

This PR provides a tentative implementation of the SSR hydration RFC

Tests for this functionality: #2541 (currently under review)

Functionality by commit:

Does this PR introduce breaking changes?

  • No, it does not introduce breaking changes.

@caridy
Copy link
Contributor

caridy commented Aug 2, 2021

Let's split this PR into two PRs, one for the wire, and one for the rehydrate.

@caridy
Copy link
Contributor

caridy commented Aug 2, 2021

Additionally, the whole point of the SSR is to accelerate the consumption of data in the UI, it is not to be correct or complete. Based on that premise, the client MUST do a full patch cycle while trying to reuse as much content as possible from what the server produced. In other words, the SSR is supposed to produce content that can be "enhanced" by the client side.

A good invariant here is that "content produced by the client side only solution must always be equivalent to content produced by the SSR + client side".

My recommendation is to run a routine that can generate (based on the template keys) an old vnode tree, and then patch that with a newly generated vnode tree, that way you can reuse as much as possible while the invariant from above can be fulfilled.

How to generate the old vnode tree? Well, I believe the easiest simplest way to do it is to generate a vnode tree that has no props, attrs or handlers, and only contains the proper vnode key, and the elem associated to it. Can that work? I don't know, we have a lot of shortcuts in the algo, e.g.: If you have classNames, you must always have an object that represents them in both sides of the diffing (old vs new vnode), that might be a problem on itself, but solvable.

packages/@lwc/engine-core/src/framework/api.ts Outdated Show resolved Hide resolved
createElmHook(vnode);

// hydrate children hook
hydrateElementChildrenHook(vnode);
Copy link
Member

Choose a reason for hiding this comment

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

The hydrate hook is not in charge of recursively invoking children hydrate if the diffing algo's job.
This hook should only be in charge of upgrading a plain element node with props and event listeners that can't be serialized in HTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't completely understand this statement, can you elaborate more?

the elements are not run through the diffing algo (there's nothing to compare to), hydrateElementChildrenHook is the one "diffing"/"associating" the elements with the vnodes. I think this is the same approach of the diffing algo 🤔 .

@jodarove jodarove changed the title poc: ssr client rehydration + enable wire adapters poc: ssr client rehydration Aug 26, 2021
@jodarove jodarove force-pushed the jodarove/ssr/poc-wire-rehydrate branch from ead8623 to f1149c5 Compare September 27, 2021 19:29
@@ -164,7 +164,7 @@ export function createStylesheet(vm: VM, stylesheets: string[]): VNode | null {
for (let i = 0; i < stylesheets.length; i++) {
renderer.insertGlobalStylesheet(stylesheets[i]);
}
} else if (renderer.ssr) {
} else if (renderer.ssr || renderer.isHydrating) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good solution! Might be good to add a comment explaining, though.

Copy link
Contributor

@divmain divmain 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! Only minor comments.

@@ -86,6 +90,26 @@ const TextHook: Hooks<VText> = {
insert: insertNodeHook,
move: insertNodeHook, // same as insert for text nodes
remove: removeNodeHook,
hydrate: (vNode: VNode, node: Node) => {
if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line lwc-internal/no-global-node
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to confirm my understanding: it is safe to disable this rule because hydrate will never be invoked during unload event, as described in #2472. Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, is safe because hydration only will be for engine-dom (so Node will be defined), and this another question/todo I have, is that IMO the hydrate implementation should live in the @lwc/engine-dom (instead of engine-core, where is today). but it can be done after this pr is merged.

packages/@lwc/engine-core/src/framework/api.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/api.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/hooks.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/hooks.ts Outdated Show resolved Hide resolved
This commit adds:
- Bailout mechanism when hydrating an element.
- Adds logic to match an element and a vnode to know if it can be hydrated.
- Runs the renderedCallback (missing from previous commits)
* wip: test framework 1st checkpoint

what i don't like:
- the test (.spec) has cjs exports and needs a "def" variable, ideally, it should only need the export default and that's it.
- it modifies existing lwc plugin, because it does need to be based on the output of pepe.

wip: checkpoint 2, a lot better

wip: checkpoint 3

wip: try run it in ci

fix: ci

fix: headers check

fix: disable safari and firefox

fix: bring back safari and firefox

* wip: add hydration test commands to the readme

* test(hydration): directives

* test(hydration): component lifecycle

* test: inner-html directive

* wip: mismatch tests

* wip: rename folder

* fix: diff and reuse dom from lwc inner-html

* wip: review feedback, better errors

* refactor: use getAssociatedVM for hydration instead of getAssociatedVMIfPresent

* fix: resolve todos

* fix: display all attribute errors at once
@jodarove jodarove force-pushed the jodarove/ssr/poc-wire-rehydrate branch from c880cb9 to 0b31e4f Compare October 26, 2021 20:48
@jodarove jodarove merged commit 647de31 into master Oct 26, 2021
@jodarove jodarove deleted the jodarove/ssr/poc-wire-rehydrate branch October 26, 2021 21:14
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.

5 participants