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

Hydration is incorrect as some attributes are missing in the SSR HTML #34

Closed
steveworkman opened this issue Nov 18, 2022 · 13 comments · May be fixed by #35
Closed

Hydration is incorrect as some attributes are missing in the SSR HTML #34

steveworkman opened this issue Nov 18, 2022 · 13 comments · May be fixed by #35
Labels
bug Something isn't working

Comments

@steveworkman
Copy link
Collaborator

Describe the bug

Given a simple element with a label

<my-button label="one"></my-button>

This renders on the server without the label attribute as it's been set as a DOM property.

When hydration comes around client-side, the component does not know what the value of label is as it's not been set as an attribute on the HTML

Expected behavior

Attributes needed for hydration should be rendered in the HTML response

Additional context

This could be a component issue, as setting a property to reflect: true resolves this for that component. However, I don't think that's reasonable for things like a label on a button. There's a lot of information in https://lit.dev/docs/components/properties/ but it's not clear what should happen with SSR.

@steveworkman
Copy link
Collaborator Author

I believe we can make it a little better by doing something like this

attachPropsToRenderer(): void {
      const customElementConstructor = getCustomElementConstructor(this.litElementTagName);
      const props = this.litElementVnode.props;

      if (props && this.renderer) {
        for (const [key, value] of Object.entries(props)) {
          // check if this is a reactive property
          if (
            customElementConstructor !== null && // The element exists
            typeof customElementConstructor !== "string" && // It's not just a string
            key in customElementConstructor.prototype && // The property we're looking for is on the prototype
            ["boolean", "function", "object"].includes(typeof value) // It's a value that doesn't render as an attribute
          ) {
            this.renderer.setProperty(key, value);
          } else {
            this.renderer.setAttribute(key, value);
          }
        }
      }
    }

This solves the basic issue, and the accordion component in the playground does not need to be reflected any more, but I'm not sure it's perfect.

@steveworkman steveworkman added the bug Something isn't working label Nov 18, 2022
@augustjk
Copy link
Contributor

I don't think the typeof check is necessarily the right thing. Rather, if the key is not in the element prototype, it's not something the element is expecting to get as a prop so it should be set as attribute, perhaps with String(value).

Maybe it's also a matter of how Nuxt handles props/attributes for custom elements. I see only attributes are added as props here, would it make sense to pass in all props here?

render() {
const attrs = this.getAttributesToRender();
return h(this.litElementTagName, {
innerHTML: this.litSsrHtml,
...attrs
});

@steveworkman
Copy link
Collaborator Author

I don't think the typeof check is necessarily the right thing. Rather, if the key is not in the element prototype, it's not something the element is expecting to get as a prop so it should be set as attribute, perhaps with String(value).

Maybe it's also a matter of how Nuxt handles props/attributes for custom elements. I see only attributes are added as props here, would it make sense to pass in all props here?

render() {
const attrs = this.getAttributesToRender();
return h(this.litElementTagName, {
innerHTML: this.litSsrHtml,
...attrs
});

This broadly seems to work in the cases we have so far. I'm going to add more tests to see if I can cover the range of what is going on

@prashantpalikhe
Copy link
Owner

I think rendering out properties (that may or may not be reflected in attributes) as attributes always may cause issues related to #30

I think the underlying problem here is that the client-side render of the Lit component seems to happen incorrectly. When Vue/Nuxt renders the component like <my-button label="one"></my-button> on the client, it should set the label property to the web component correctly (I think this is not happening for an unknown reason). If that is done correctly, then Lit's hydrator should have the correct render result when it invokes the render on the patched Lit component.

@augustjk correct me if my hunch is wrong here

@augustjk
Copy link
Contributor

Hmm.. I think in principle that makes sense. For server rendering, setting property on the element only makes sense for generating the shadow dom, and the generated markup of the element should just contain the attributes, including any reflected properties.

As @prashantpalikhe pointed out, the problem could lie more in how vdom node on the client side maps to the custom element. We did notice the vdom node did have all the props but wasn't setting them on the actual custom element on the client once loaded. Perhaps the difference in client/server behavior of the wrapper confuses Vue/Nuxt that it doesn't know the custom element generated from the h(this.litElementTagName, ...) is meant to be the target to add props to?

@steveworkman
Copy link
Collaborator Author

I've added a test component based on one we have at Maersk that is showing the hydration and SSR issue in #35 (my-step-indicator).

The outer component, which was wrapped in the LitWrapper is rendered, but the inner component does not have its shadowroot filled, despite the render method being called with the correct properties. I'm not sure if this is an issue with the lit renderer or the way we're passing properties, but it is showing up as a hydration issue int he console.

The first error says there should be exactly one root part in a render container but in this case there are zero elements. There are three of them as we are trying to render three<my-step-indicator-item> elements

The second error message is a side-effect of this as it's trying to hydrate something that doesn't match.

image

@augustjk
Copy link
Contributor

The hydration error I think steps from the conditional render in my-step-indicator. If this.labels might have the 3 items in one environment and defaulting to empty array in another environment. Does the server rendered declarative shadow dom contain the 3 items? If so, then it might be Vue failing to pass the props to the correct element before hydration.

@steveworkman
Copy link
Collaborator Author

@augustjk yes, in this case labels is [] client-side - it's passing through perfectly well server-side. Changing the property to a string and splitting it with commas works without any issue. It also looks like it is specifically Arrays - I was playing with it and found that passing in an object of { labels: ['label'] } passed the outer object through, but not the Array!

@augustjk
Copy link
Contributor

Is it specifically Vue/Nuxt not being able to provide array props to the element? That's odd!

@prashantpalikhe
Copy link
Owner

prashantpalikhe commented Nov 22, 2022

When looking a bit into the hydration issue earlier, I had noticed a strange situation where the custom element was instantiated was Vue and therefore passed in all the props while NOT using the SSR. But while using the SSR, the custom element was instantiated as soon as the custom element definition arrived on the browser. And the call stack showed nothing else. The first call in the call stack was customElement.define(...). So the custom element got instantiated with no props. That maybe the cause of this issue. No idea yet why it behaved differently. Maybe presence of the DSD?

UPDATE: This was not related to the issue. But did help me eventually get in the right direction. Check my comment below for the newer findings.

@prashantpalikhe
Copy link
Owner

@steveworkman @augustjk

So I dived into this further, investigating how Vue applies props to the custom elements while hydrating an SSR-ed application. And I found out that Vue does not apply props to components during hydration.

https:/vuejs/core/blob/9698dd3cf1dfdb95d4dc4b4f7bd24ff94b4b5d84/packages/runtime-core/src/hydration.ts#L329

You can see here that only props named value or starting with on are applied/patched.

IMO, this is the reason why we noticed that during Lit's hydration, the Lit components were instantiated without correct values (only default values).

We got around this issue by rendering all props as attributes. But I think that's not always desirable (explicitly set by component authors to not reflect as an attribute) and in the case of complex data types, like array/objects, not possible either.

But if we were to apply the props for custom elements during hydration on Vue's side, then the issue gets resolved.

We do need to defer Lit's hydration until Vue's hydration is complete. But that's very simple to achieve by adding a defer-hydration attribute in the SSR-ed Lit component and removing it when Vue mounts the Lit component on the client side.

I tried @steveworkman 's branch by making these modifications, including patching Vue's source code to apply props for custom elements during hydration, and the issue is resolved. Would be great to discuss this further with a Vue dev. to get their insight into this.

@prashantpalikhe
Copy link
Owner

@steveworkman I think this issue can be closed now, now that Vue has fixed the bug, right?

@steveworkman
Copy link
Collaborator Author

Yes, fixed in 3.4.36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants