[later phase] Unwrap `<astro-island>`s and `<astro-slot>`s

Upgrading this to a beta blocker, because we want early adopters of the beta able to run XB in production without having their CSS break when we eventually add SSR.

I discussed this with @lauriii and according to him it's sufficiently rare to have CSS that's finicky to whether the wrapper tags are there or not that he's okay with this being an area where we break BC between beta and RC.

Took a stab at this.
I was able to simplify the 'unwrapping' code to register a second custom element and avoid the proxy.
I also didn't need to track 'data' dependencies because we only need a check for `'use client';` in ::renderComponent and at that point we have the JS in scope, so we can just check that directive exists then.

Working well locally. Added a playwright test that verifies the slots are being removed - which required adding a few more test components.

Ready for another review

"Needs work" for the feedback above, but also a more philosophical question related to it:

If someone uses useState(), then they need to explicitly add 'use client';. That makes sense and I think is a standard requirement for any React codebase that uses RSC. However, are we okay that component authors will also need to add 'use client'; if their component calls useSWR() even if it has no other client-side code? In principle we could eventually add support for useSWR() to be SSR'able. But until we do, is it reasonable for component authors to have to know that our current implementation isn't and that's why they need to add 'use client';?

I considered whether we should make #unwrap check for either 'use client'; or useSWR(), but the downside with that would be that if in the future we do make useSWR() SSR'able and then change #unwrap back to just be based on 'use client'; only, then that would be a BC break for existing components, since components that we were previously keeping wrapped with <astro-island> would start getting unwrapped, so existing site CSS could break for these. Therefore, I think keeping #unwrap tied to only 'use client'; is preferable in that component authors will be in control of if/when to remove their own 'use client'; directives on a component by component basis, once we add the ability for useSWR() to be compatible with SSR'd components.

I guess my questions are:

1. Does my reasoning above make sense?
2. Even if it does, are we concerned about it being a WTF for component authors in the meantime needing to add 'use client'; for components that aren't interactive but need to fetch data?

Other than #9, I think this MR looks great.

Given how close this MR is, tagging it as a beta target, because doing it after beta1 does involve a small BC break of site CSS, even if rare, per #3.

I would definitely not have expected useSWR() to require using 'use client'. useState() and attaching event listeners is what I would have expected to require that. I have no idea what to do about that – I have a feeling that complexity is creeping in.

This also breaks components that use import Image from "next-image-standalone"; because internally that makes use of state.

That's a non-starter

I would definitely not have expected useSWR() to require using 'use client'.

Same. But according to https://swr.vercel.app/docs/with-nextjs, useSWR() is intended as a client API and is not supported by server components. And when I google search "does useSWR require 'use client' directive", Google's AI response is "Yes, useSWR requires the 'use client' directive in your component file". So us requiring it wouldn't break existing React ecosystem expectations with respect to this.

Went with auto-detection of hooks and use of the next-image-standalone component instead of requiring 'use client'; this gives us some flexibility for now.

I feel like I missed a trick not naming the element <phantom-island>

I have a feeling that we need to think about this problem more holistically. It seems like we all had some expectations how this would work and it's clear it won't work the way we expected it to work. I think we should do an ADR that documents the different use cases we need to support which would then allow us to document how we want this to work and how we get to a reasonable experience. Something I think may reduce the urgency here is that we could consider all components prior to SSR support as 'use client' components, so we'd not try to add SSR to any components secretly.

The back-end changes are trivial. So, prior to merging, I wanted to check this with @jessebaker, because this is a front end-heavy MR.

Discussed it with him. He's tested this. He's reviewed it. It unblocks many other issues. Worst case, we could refine what this brings.

I'll land this after #3503412: Allow Content Author to set site's homepage from navigator, which is the more urgent issue that in principle is unrelated, but I don't want to risk delaying that more complex MR by merging this one first.

🤓

I think auto-detection should be provided as a convenience layer instead of our default way of deciding when to unwrap: a best effort safety net in case the code component author doesn't explicitly express that the code component is using client-side code (i.e. by adding the 'use client' directive in the code or setting a config entity property), and we're able to catch when the component should be treated as such. I say best effort because of two reasons:

1. We won't be able to examine code brought in via URL-based imports.
2. In the future we would like to support code components created via the CLI tool using custom JS compilation to open the door for writing components in other frameworks than React.

I also left a comment on the regular expression finding all hooks, because I don't think all hooks should prevent us from doing the unwrapping.

I hate to put this back to NW, but I don't feel like we completely cracked this yet. #1 above means that if we were to land the issue without changes, we'd introduce a regression. One of the cool ways of building things with XB right now is to import packages from, e.g., https://esm.sh. (It's also something we just nicely documented in #3535089: Move code component starter template documentation from inline comments to online docs .) We will try to unwrap anything that will be imported that way due to the auto-detection not looking into that code, which can break in many ways.

I have an alternate idea here.

The reason this is forcing us to chose between unwrapped and not is because we're treating each component as its own island and the unwrapping code is doing insertBefore rather than moveBefore which means we're losing events etc. moveBefore is only supported in chromium based browsers so we can't use that.

I think we can combine this and #3514910: Refinements of real-time preview for props changes of JS components if we expand the size of our island to include the whole preview, rather than one island per component.

With the wrapping islands we have the css impact (that we're working around with display: contents now). Without them we lost interactivity and also make things harder for #3514910: Refinements of real-time preview for props changes of JS components.

But if instead we have one island we can retain the wrapper and the interactivity and possibly make #3514910: Refinements of real-time preview for props changes of JS components simpler.

I'll do a spike on that approach to see if it's viable. In researching #3514910: Refinements of real-time preview for props changes of JS components I've read the internals of astro and the preact client, there's not much code there, I am confident we can fashion something that works for us.

I did a short spike on #26 and whilst I think it is feasible, it would bind us to too tightly to preact and the internals of astro which would make supporting other frameworks more difficult in the future.

As per above I think we need to document all the use-cases here.

Astro uses the display contents approach, maybe that's where we leave this - i.e. we mark this as closed won't fix.

I'd love to see a proof of concept of #26. I'm curious how you manage to get around the need for some kind of wrapper element for each component. React's createRoot() and createPortal() both require a DOM element container that they can completely take over. Does your idea somehow work without any such wrapper element? If so, even if it's bound to Preact and Astro internals, can you elaborate on the approach? Maybe we can find a way to implement a similar approach with the other Astro-supported JS frameworks as well?

I think the problem with #28 is that display: contents doesn't affect CSS nth-child selectors and similar. So the presence of <astro-island> elements isn't completely transparent. It's basically the <div> soup problem, just with <astro-island> instead of <div>. Ideally, XB wouldn't insert any extraneous markup at all into the DOM, so that component authors are in 100% control of their markup.

I think it's okay for client-interactive components to have <astro-island> wrappers: Astro is a popular framework, so this is easy to explain and point people to docs and blog posts about it. But for non-interactive components, Astro would normally SSR (or SSG) those, and ideally so will we once we add optional SSR support to XB, so the goal of this issue is to output a DOM that's the same as what we would end up having with SSR.

Sorry, didn't mean to change the issue attributes.

A specific (though contrived) example... it should be possible to make a List Code Component that does:

return(
  <ul>
    {item1}
    {item2}
  </ul>
)

And for item1 and item2 to be slots that get populated with ListItem components that do:

return(
  <li>FOO</li>
)

And so long as these are non-interactive components, for the DOM result to be:

<ul>
  <li>FOO</li>
  <li>FOO</li>
</ul>

Instead of:

<ul>
  <astro-slot><astro-island><li>FOO</li></astro-island></astro-slot>
  <astro-slot><astro-island><li>FOO</li></astro-island></astro-slot>
</ul>

#31 is probably contrived in that why wouldn't the component author just move the <li> tags from ListItem into List, which would then be more robust to the item slots getting populated with anything, but there are probably real use cases out there that are less contrived, so #31 is just intended as illustrative.

both require a DOM element container that they can completely take over

I've blown away my local work but should be able to revive it from phpstorm local history.

my idea was to add an xb-preview custom element at the root of the ComponentTreeItemList::renderify call and use that as the container that is taken over.

I replaced <astro-island> with a <template data-xb-island> element but retained all the attributes, after renaming them to have a data- prefix. I also changed the <template data-astro-slot> to be data-xb-slot

In the custom xb-preview element I was planning on using querySelectorAll to find all <template data-xb-island> and then looping over those to Promise.all on their imports.

I needed to copy astro-island's 'reviving' of properties wholesale, but that's not a lot of code.

Where the hard-coded importing of astro's preact crept in was the renderer's 'static-html.ts' - and of the h function although I note that they also have similar functions for other their other framework renderers.

The idea was to build a single component tree with the SDC and block components alongside the JS components and then use the preact render call on that inside the top level xb-preview custom element. Its a bit like what we're doing with hyperscriptify but on the preview side.

If you think this is worth exploring further I'd be happy to, I timeboxed it to ~90 mins because I didn't want to go too far down a rabbit-hole if the result was too rigid/limiting of future options.

I could see us having a factory of renderers based on the framework option, it does appear that most of Astro's renderers follow a similar pattern.

I love the creative thinking in #33, but some questions/thoughts:

The idea was to build a single component tree with the SDC and block components alongside the JS components and then use the preact render call on that inside the top level xb-preview custom element. Its a bit like what we're doing with hyperscriptify but on the preview side.

Are you thinking we only do this when we're previewing inside of XB? Or when viewing the page on the published site outside of XB as well? If the former, I'm concerned about the potential for the preview to lose accuracy if it's following a significantly different rendering path vs. viewing the page on the published site. If the latter, I'm concerned that having the whole page (including SDCs and Blocks) running inside of a single Preact VDOM tree ala hyperscriptify isn't sufficiently battle tested (us dogfooding it within XB UI for the right sidebar does count for something, but I don't think that gives us enough confidence in it to force it on people outside of XB UI) and might not be desired by people wanting to adopt JS components gradually. The advantage of Astro is that it's a mature and widely adopted framework where a lot of the potential bugs with nesting and intermixing interactive islands and static HTML have already been worked out.

The reason this is forcing us to chose between unwrapped and not is because we're treating each component as its own island

I don't see it this way, or rather, I don't see it as a negative. The way I see it, Astro is a well-loved framework for building content-driven sites, exactly the kind of sites XB is for. XB "just" adds the bonus of a really good UI and really good integration with a really good CMS. When building in Astro, you choose which components are server rendered (by default, all of them) and which are rendered as client islands (via a client:* prop/attribute/directive). The former get rendered without any wrapper markup. The latter get rendered with the <astro-island> and <astro-slot> wrappers. Given how successful and well maintained Astro is, I don't really see any of that as a problem that we need to improve upon.

The unique thing introduced by XB is that within XB, people aren't writing .astro files by hand, so we don't have client:* directives to tell us which component instances need to remain wrapped. But we don't need to re-invent that from scratch either: React already defines the idiomatic way to do that for React components: the 'use client'; directive.

I think the earlier MR here (before I derailed us with comment #9) was on the right track. However, I think the logic needs to be: if the component has the 'use client'; directive, or if one of its dependencies does. But that needs to include dependencies that we're already tracking (i.e., other code components) as well as import dependencies that we're not yet tracking (swr and next-image-standalone). So perhaps the front-end could add some logic to add 'use client'; to a code component that imports either of those two? So the backend only needs to check for 'use client'; on the code component or its code component dependencies?

the unwrapping code is doing insertBefore rather than moveBefore which means we're losing events etc. moveBefore is only supported in chromium based browsers so we can't use that.

Yes, I can see why my original code that's in the issue summary is naive, since elements get inserted to the DOM, then removed, then reinserted. What a shame that moveBefore() isn't available on FF and Safari. But a new realization I just got is that ultimately what we're trying to emulate is SSR. So, what if we make <unwrapped-astro-island> emulate that better, by using renderToString() (same as server.js within @astrojs/preact) and then replacing its .outerHTML with that? This would result in the rendered HTML getting inserted into the DOM just once.

Are you thinking we only do this when we're previewing inside of XB? Or when viewing the page on the published site outside of XB as well?

I was doing it for both. I hadn't finished wiring up <xb-preview> yet, but the SDC elements were working as-is (as you'd expect). I'd still like to explore this approach as it sidesteps the need to detect use client and unwrapping altogether.

I think the earlier MR here (before I derailed us with comment #9) was on the right track. However, I think the logic needs to be: if the component has the 'use client'; directive, or if one of its dependencies does.

Ok, sounds like this is the next step for this issue.

I just got is that ultimately what we're trying to emulate is SSR. So, what if we make emulate that better, by using renderToString() (same as server.js within @astrojs/preact) and then replacing its .outerHTML with that? This would result in the rendered HTML getting inserted into the DOM just once.

Yep, we could do that but we'd still have to make the decision about interactivity with use client.

So it sounds like the next logic steps here are:

1. Change the logic back to detect 'use client', but also consider dependencies AND useSWR or next-image-standalone as a trigger for preventing unwrap
2. Explore using renderToString to replace the outerHTML rather than rendering and then moving

The questions is, how much of this is moot if we have SSR? Should we be focusing on that instead? I think we can make that pluggable so that there are multiple SSR providers - e.g. symfony/process that calls node on the host, a HTTP endpoint that calls a separate container running an express web-service etc.

Going to pause and create an ADR around the requirements here and for SSR

ADR/discussion/research on SSR is here for review

We actually have a way to workaround this, which is to mark all existing components as 'use client' when we introduce this. Because of that, changing this to stable target instead.

@jessebaker: is this issue still relevant?

I think we should postpone this until we figure out SSR. When we're ready to start working on SSR, we should open a meta issue for it. This issue's MR 1363 could then become its own child of that issue. MR 1363 is a great start but still needs more evolving to make sure we're creating the right DX/UX for code component authors.

WFM, thanks.

(Working my way through our massive and quite stale Needs review queue!)