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

bug: Hydrate: serializeShadowRoot: false does not render content of the shadow dom of components. #5924

Closed
3 tasks done
mayerraphael opened this issue Aug 5, 2024 · 6 comments · Fixed by #5927
Closed
3 tasks done
Assignees
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@mayerraphael
Copy link

mayerraphael commented Aug 5, 2024

Prerequisites

Stencil Version

4.20.0

Current Behavior

When using serializeShadowRoot: false, the contents of the components are incorrectly rendered. Slot projection seems completly broken.

With Stencil 4.19+

For some components, the full render() method content is missing, only the slotted object is there with no slot:

image

Same component with Stencil 4.18.3

image

There are also another problem with the renderToString options.

const { html } = await stencil.renderToString(appHtml, {  }); 

This renders like setting serializeShadowRoot: true explicitly even though default is false. But null seems not to be equal to false here. The option is nullable.

image

serializeSHadowRoot must be explicitly set to false to use the old, but broken, behavior.

Expected Behavior

The old hydration concept to work fine again. Tested against 4.13.0 and 4.18.3, which works fine.

System Info

Stencil 4.20.0 with React Output Target 0.5.3. (latest stable).

Steps to Reproduce

Create a component with a (default) slot:

render() {
    return (
      <Host>
        <div>
          <slot></slot>
        </div>
      </Host>
    );
  }

Render the component with slot content:

<MyWhateverComponent>Abc</MyWhateverComponent>

The generate html is broken or the html of the components shadow dom is missing completely.

<my-whatever-component class="sc-my-whatever-component-h hydrated" s-id="1">
  <!--r.1-->
  Abc
  <div class="sc-my-whatever-component" c-id="1.0.0.0">
     <slot class="sc-my-whatever-component" c-id="1.1.1.0"></slot>
  </div>
</my-whatever-component>

See nextjs-app-pagerouter example in my repository: https://github.com/mayerraphael/stencil-dsd-ssr-playground

Code Reproduction URL

https://github.com/mayerraphael/stencil-dsd-ssr-playground

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Aug 5, 2024
@christian-bromann christian-bromann added Bug: Validated This PR or Issue is verified to be a bug within Stencil and removed triage labels Aug 5, 2024
@christian-bromann christian-bromann self-assigned this Aug 5, 2024
@christian-bromann
Copy link
Member

christian-bromann commented Aug 5, 2024

Thanks for reporting the issue. To clarify the issue: given a shadow component (with shadow: true flag set), if I render the component to string, e.g.

const { html } = await renderToString(`<my-whatever-component>Abc</my-whatever-component>`, {
  fullDocument: false,
  prettyHtml: true,
})

console.log(html);

I expect the slot to be replaced with the actual light dom, e.g.

    <my-whatever-component class="hydrated sc-my-whatever-component-h" s-id="1">
      <!--r.1-->
      <!--o.0.1.-->
      <div c-id="1.0.0.0" class="sc-my-whatever-component sc-my-whatever-component-s">
        <!--s.1.1.1.0.-->
        <!--t.0.1-->
        Abc
      </div>
    </my-whatever-component>

however the last stable of version of Stencil returns:

    <my-whatever-component class="hydrated sc-my-whatever-component-h" s-id="1">
      <!--r.1-->
      Abc
      <div c-id="1.0.0.0" class="sc-my-whatever-component">
        <slot c-id="1.1.1.0" class="sc-my-whatever-component"></slot>
      </div>
    </my-whatever-component>

@christian-bromann
Copy link
Member

Thanks for reporting the issue and apology that we didn't clearly communicated this change. So let me do this here:

As we've further improved support for declarative Shadow DOM, we've realised that it wasn't feasible for us to allow users to render a shadow component as scoped component after the component has been compiled. The reason is that Stencil compiles the styles for either rendering a component is shadow or scoped mode. As part of the compilation step Stencil puts these compiled styles into the hydrate module at which point they can't be transformed anymore to support the other mode. Knowing that this would introduce a breaking change for this particular feature, we've decided as a team to move forward with it and introduce the change as part of v4.19.0 (see PR #5892).

Our recommendation moving forward is to serialize all components marked with shadow: true as a declarative shadow DOM.

It would be super helpful if you could explain your use case and reasoning why it is beneficial for you to serialize shadow components as scoped components. Supporting both modes post compilation will require a larger engineering effort which I doubt we have capacity to execute on but having a use case example could make a case for it.

@mayerraphael
Copy link
Author

@christian-bromann We have a very large project and we wanted to deploy it twice to measure performance and other aspects (like seo etc) of the old and with the new rendering to have a direct comparison. It would be just for the transition phase.

But thanks for the clarification. We ofc understand that the support for both will be a pain.

@christian-bromann
Copy link
Member

Thanks, let me go ahead and close the issue then once the PR with the additional documentation is merged.

One note on the performance side, we have tested the Next.js support on a large design system and I've discovered the following observation: we created a test page where we had all components of the system being rendered in different variations, about 200 of them. This obviously had an impact on the initial page load time, not critical but also probably not ideal. We would like to gather more information about real world use cases, so if you have any information for us, please share them if you can. There are certainly optimizations we can make but given our limited resources we want to ensure they are impactful.

@mayerraphael
Copy link
Author

mayerraphael commented Aug 7, 2024

@christian-bromann

DSD (declarative shadow dom) with an rendered style tag kinda only works if not the full page is rendered. Especially deeply nested components like navigations etc with lots of items bloat the payload immensly.

We have 152 individual components and a normal product page has over 650 rendered components.

One page just changing from old to new concept increased the payload from 2.4MB to 23MB uncompressed.

Thats why our current project needs time to migrate to the new Stencil Version / DSD rendering and they will still stick for some time to the old rendering for now. Each page needs to be evaluated and opimized with what is rendered on the server and what can be moved to the client to keep the style bloat minimal.

Our future concept is, like AboutYou, to only render the initial viewport on the server and the rest of the data may be there (in NextJS' data json), but is rendered on the client only to keep the load on the server and payload small.

I still think DSD is the better solution than the old scoping/descoping. We have better block time. It is a web standard.

What possible solutions are there:

https://developer.mozilla.org/en-US/docs/Web/API/Web_components/Using_shadow_DOM#applying_styles_inside_the_shadow_dom

Maybe we need both techniques? Something like:

@Component({
  tag: 'my-component',
  styleUrl: 'my-component.scss',
  shadow: true,
  styleHydrationMode: "declarative"  (or "constructed")  // you may find a better name for that :)
})
export class MyComponent {

There are discussions here:

Currently there is no declarative solution which works on parse time (like DSD). All solutions to share CSS require JS.

@christian-bromann
Copy link
Member

A fix for this bug has been published in Stencil v4.21.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants