withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
46.65k stars 2.48k forks source link

[astrojs/lit] web components don't adopt SSR'd DSD from view-transitions #9953

Closed hrmcdonald closed 8 months ago

hrmcdonald commented 9 months ago

Astro Info

Astro                    v4.2.8
Node                     v18.19.0
System                   macOS (x64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             auto-import
                         @astrojs/lit
                         @astrojs/mdx
                         @astrojs/react

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Our design system is built with Lit web components. Our docs site is built with Astro. We use the @astrojs/lit plugin to SSR all our components on pages and it works great.

However, recently we have been testing out Astro's newer view-transition functionality and we noticed a major issue going on here that is blocking us from adopting this functionality right now.

While the initial page load works as expected, any SSR'd web components loaded in a view-transition do not adopt their SSR'd shadow root. I was pointed to the fact that the browser's declarative shadow DOM parsing only runs during HTML parsing. I don't know enough about view-transitions or how Astro handles them to be able to know if this is why these DSD roots are not getting adopted properly. Is this a browser bug? Should or could it be handled by Astro?

What's the expected result?

In the example I have linked below, I have created a very simple Astro site with two pages, a web component on each, and view transitions enabled.

The first page you load (can be either one) everything parses and renders as expected. But once you click the link to transition in the other page, you will notice the second page's web component link does not render correctly. It is there, but it has not adopted its SSR'd shadow DOM and thus the link on the second page is not visible.

Initial load:

image

Page when loaded via view-transition:

image

Ideally, we want web components loaded during a view transition to get parsed like normal so they adopt their DSD. Otherwise we can't really utilize view transitions with SSR'd web components.

Link to Minimal Reproducible Example

https://stackblitz.com/~/edit/withastro-astro-ytwbdv?file=src%2Fpages%2Findex.astro&title=Astro%20Starter%20Kit:%20Blog

Participation

martrapp commented 9 months ago

Hi @hrmcdonald, thank you for opening this issue!

It looks like we are not putting the template in the shadowRoot when we parse the new DOM from the file.

martrapp commented 9 months ago

The problem only seems to occur in development mode. @hrmcdonald, could you please check/confirm if it works for you with build/preview?

hrmcdonald commented 9 months ago

It does appear that things are working properly in a production build.

martrapp commented 9 months ago

Still a bug so. Have to dive deeper into hydration. Any suggestions welcome. Will ask on discord, too :)

martrapp commented 9 months ago

Preliminary analysis: For view transitions, we load the DOM with DOMParser.parseFromString(). No special attention is paid to templates and we have no special handling for shadowRoots. At first I thought that DSD support was simply missing, but then I got confused by the fact that it works with astro build. Maybe it's just a matter of re-initializing after swap. To be continued ...

augustjk commented 9 months ago

FYI declarative shadow DOM won't be parsed with DOMParser.parseFromString(). Chrome shipped the ability to do so with a {includeShadowRoots: true} option passed in but it was before the spec was finalized. The correct way to parse string to a document while also letting the browser parse and attach the declarative shadowroots would be to use Document.parseHTMLUnsafe() which was added to the spec here https://github.com/whatwg/html/pull/9538.

If you don't need an entire document and would like something similar to setting .innerHTML that also parses DSD, Element.prototype.setHTMLUnsafe() has also been added to the spec. Here's an example of it in use with a feature detection and fallback to a ponyfill: https://github.com/lit/lit/blob/31c181a8a225b807719a3e52b980bf02dcb64255/packages/labs/testing/src/lib/fixtures/ssr-fixture.ts#L75-L86

Currently the unsafe HTML methods are available in the recently released Firefox 122, and soon to come Safari 17.4 (currently available in Safari Technology Preview). Chrome is in the process of implementing.

martrapp commented 9 months ago

Hey @augustjk!

Thank you so much for your detailed explanation and advice! I really appreciate it!

Some how, we get things right when building for production. Let me learn some more about Lit 😄

hrmcdonald commented 9 months ago

Actually, when inspecting the production build a bit closer, I don't think things are working as expected still. It seems that in a prod build for some reason the web components are just re-initializing themselves properly when loaded via view-transition where they don't in dev. However, they still don't seem to be properly adopting their DSD template.

The stackblitz example is simple enough that it looks OK on the surface, but for anything more complex this still leads to a lot of issues.

martrapp commented 9 months ago

Took a while. Several thing come together here

github-actions[bot] commented 9 months ago

Hello @hrmcdonald. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with needs repro will be closed if they have no activity within 3 days.

hrmcdonald commented 9 months ago

I updated the repro stackblitz to use link instead of href as a prop. I'm not sure what to recommend as the best path forward though from here. We can hold off on utilizing view transitions until browser support is better here.

martrapp commented 9 months ago

That at least changed the error message for href to link. ;-) image I am afraid that as long as there are uncaught exceptions in the browser console, they will prevent the shadow root from initializing correctly. I have no experience with lit and can't really help with fixing the lit problem. To get rid of the original error, I simply deleted the @property() href: string; and hardcoded the href in the anchor. But then the issue disappeared for me too.

Please help me with a reproduction that does not throw errors in the browser console during loading or view transitions.

github-actions[bot] commented 9 months ago

Hello @hrmcdonald. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with needs repro will be closed if they have no activity within 3 days.

martrapp commented 9 months ago

Please note that the uncaught error also occurs independently of view transitions. If <ViewTransitions /> is removed from Base.astro, the error will show up on page load.
So it's not so much a question of missing browser support for view transitions, but we should figure out how to fix the basic lit problem here.

martrapp commented 8 months ago

@hrmcdonald here is the stackblitz for a minimal reproducible example that works for me. https://stackblitz.com/c/edit/withastro-astro-wmttqt?file=src%2Fcomponents%2Fmy-custom-button.js&title=Astro%20Starter%20Kit:%20Blog It doesn't throw errors, sets the shadow DOM, and shows working view transitions. Testes on Chrome Version 121.0.6167.185 (Official Build) (64-bit).

Does that work for you, too? Do you see any further issues?

martrapp commented 8 months ago

@hrmcdonald I'm closing this issue now as there was no further response. Please feel free to reopen it at any time if you gain new insights.

hrmcdonald commented 8 months ago

Sorry for the delayed response here @martrapp. I don't know why the error you called out is getting thrown with decorator properties in the example, but even in your example that fixes that issue, while it looks like things are working properly, the web component is not initializing properly. Its SSR'd template shadow root is not getting properly adopted:

image

See how the SSR'd declarative shadow dom template is still orphaned here. It looks like the web component is just initializing from scratch instead of adopting the SSR'd shadow dom template like it should.