Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Server-side rendering (SSR) compatibility #778

Closed
augustjk opened this issue Jun 3, 2022 · 25 comments
Closed

Server-side rendering (SSR) compatibility #778

augustjk opened this issue Jun 3, 2022 · 25 comments
Labels
feature Feature requests. help wanted Ready for a contributor to tackle.

Comments

@augustjk
Copy link

augustjk commented Jun 3, 2022

Hello!

I've been testing out a work in progress utility for testing Lit elements being rendered server-side (lit/lit#2957) and have been trying it out on some shoelace component tests. I wanted to report some initial findings for things that were breaking when putting the components through lit-ssr which were mostly due to unavailable DOM APIs.

  • focus-visible check

    export const hasFocusVisible = (() => {
    const style = document.createElement('style');
    let isSupported;
    try {
    document.head.appendChild(style);
    style.sheet!.insertRule(':focus-visible { color: inherit }');
    isSupported = true;
    } catch {
    isSupported = false;
    } finally {
    style.remove();
    }
    return isSupported;
    })();

    • Styles that import this file errors during SSR due to the style.remove() in the finally block.
    • Making it style.remove?.() lets this proceed without throwing and I believe the check runs properly when styles are updated during hydration.
  • base-path

    const scripts = [...document.getElementsByTagName('script')] as HTMLScriptElement[];

    • Importing this file errors due to the missing getElementByTagName
    • Perhaps deferring this evaluation until the first time getBasePath is called would be okay?
  • icon.ts using DOMParser

    const parser = new DOMParser();

    • Perhaps the parser could be instantiated when it's needed?
  • hasSlotController

    private hasNamedSlot(name: string) {
    return this.host.querySelector(`:scope > [slot="${name}"]`) !== null;
    }

    • When this gets called in a component's render during SSR, this.host.querySelector doesn't exist.
    • I found this issue related to this [labs/ssr] Access to slots during SSR lit/lit#1995
    • I'm not sure what the best solution for this might be. Perhaps a css solution might be possible for when slot detection is needed like suggested above?

I'll add more findings as I come across them. Thanks!

@augustjk augustjk added the feature Feature requests. label Jun 3, 2022
@claviska
Copy link
Member

claviska commented Jun 6, 2022

Thanks for diving into this! I'd love to improve things here, but I have a few questions.

Making it style.remove?.() lets this proceed without throwing and I believe the check runs properly when styles are updated during hydration.

Would it make sense to add this API to the testing tool as a placeholder method? I'm sure I'm not the only one using this, and I'm fine with using optional chains if needed, but I'm concerned that contributors and even myself will remove the ? at some point.

I can't really think of a great way to prevent that from happening in the code (short of specific comments or tests), so it would feel a lot better if the testing tool implemented it as a no-op to prevent fails. Maybe it can emit a warning if there's a potential pitfall in relying on it? 🤔

base-path - Importing this file errors due to the missing getElementByTagName

I'm OK with deferring this one. Great suggestion!

icon.ts - Perhaps the parser could be instantiated when it's needed?

I'd like to reuse the parser to avoid the overhead of instantiating a new one for each icon. Is it safe to move it to the constructor, or will that also cause the test tool to fail?

hasSlotController - When this gets called in a component's render during SSR, this.host.querySelector doesn't exist [...] I'm not sure what the best solution for this might be. Perhaps a css solution might be possible for when slot detection is needed like suggested above?

This is indeed unfortunate. I'd really like to see a CSS solution that solves for whitespace and the absence or presence of text nodes, element nodes, etc. since the browser already has that information.

I can see if there are any instances that can be solved with pure CSS, but :empty is practically useless in this case so I don't think I'll be able to change much here.

Thanks again!

@claviska
Copy link
Member

claviska commented Jun 6, 2022

One more thing...is there a list of issues/gotchas that I can reference? Knowing what not to do will be helpful, and I'd also like to link to or add it to the contribution guidelines.

@augustjk
Copy link
Author

augustjk commented Jun 6, 2022

Would it make sense to add this API to the testing tool as a placeholder method?

We could potentially add a no-op .remove() to the object returned by .createElement() in our DOM shim. @justinfagnani what do you think?

Is it safe to move it to the constructor, or will that also cause the test tool to fail?

The constructor is also run server side where DOMParser is unavailable. Perhaps it can be instantiated within firstUpdated?

  firstUpdated() {
    this.parser = new DOMParser();
    this.setIcon();
  }

One more thing...is there a list of issues/gotchas that I can reference? Knowing what not to do will be helpful, and I'd also like to link to or add it to the contribution guidelines.

We have a brief documentation of SSR limitations here https://github.com/lit/lit/tree/main/packages/labs/ssr#notes-and-limitations. We're working on expanding this more hopefully soon!

@justinfagnani
Copy link
Contributor

I'd love to find a way to SSR the icons, not just delay their parsing/render till hydration. That would probably require quite a change in the icon system though. Some of on the Lit team may go off an experiment there and see what we can come up with...

@claviska
Copy link
Member

claviska commented Jun 6, 2022

The constructor is also run server side where DOMParser is unavailable. Perhaps it can be instantiated within firstUpdated?

I suppose we could create a new instance on firstUpdate, but I’d still like to scope the reference to the module so we’re not instantiating one per icon. There’s a little gymnastics there, but a comment should make it pretty clear what’s going on.

I can do this in the meantime. I’m interested to see what Justin et al come up with, but I’m not sure how it will work without baking in all the icons. I’d really like to continue supporting multiple icon libs that fetch from anywhere.

@claviska
Copy link
Member

claviska commented Jun 7, 2022

Updates:

  • The call to getElementsByTagName() has been deferred to the first time getBasePath() is called. ce09ac2
  • The DOMParser instantiation has been moved into the setIcon() method which gets called on firstUpdate() 15dbb0a

Outstanding issues include:

  • Can the testing tool support a no-op remove() method on dynamically generated elements? Are there any ill effects of doing this, or is this a Shoelace edge case? /cc @justinfagnani This is no longer required since the focus visible shim has been removed.
  • Slot detection (I don't think this has a solution without updating the CSS spec to support something like :blank)

@claviska
Copy link
Member

claviska commented Jun 8, 2022

Today's release includes the fix for base path and DOMParser. I'm willing to solve the remove() issue if it's a blocker, but the last item (slot detection) will probably be a limitation of specific components until we have a CSS solution or a reasonable workaround. I'm open to suggestions!

@claviska claviska changed the title Server-side rendering compatibility Server-side rendering (SSR) compatibility Jul 6, 2022
@claviska
Copy link
Member

claviska commented Jul 6, 2022

Bumping this to see if we can address this one:

Can the testing tool support a no-op remove() method on dynamically generated elements? Are there any ill effects of doing this, or is this a Shoelace edge case? /cc @justinfagnani

Also, is anyone of aware of a CSS proposal that would let us remove slot detection logic? Should we start one?

@augustjk
Copy link
Author

augustjk commented Jul 6, 2022

I'll bring up adding a no-op remove() in the DOM shim to the team.

For the CSS proposal, this one seems most relevant. WICG/webcomponents#936

@wolthers
Copy link

wolthers commented Jul 7, 2022

@claviska Now that Safari supports focus-visible out of the box, you can probably remove the feature detection alltogether. https://caniuse.com/?search=focus-visible

@claviska
Copy link
Member

claviska commented Jul 7, 2022

Now that Safari supports focus-visible out of the box, you can probably remove the feature detection alltogether. https://caniuse.com/?search=focus-visible

Yes, now that the last two major versions (15 + 16) support it, the shim has been removed.

@augustjk
Copy link
Author

I'll bring up adding a no-op remove() in the DOM shim to the team.

So we are moving in a direction to reduce the reliance on these no-op DOM shims as we can't account for what the correct intention of the shim's use case would be, if no-op should even be the right thing to do.

It would be better for authors to be able to handle these more intentionally so we're looking to provide a way to do that here lit/lit#3158.

@claviska
Copy link
Member

Subscribed, thanks. I like this idea since it would give authors the ability to render a fallback or skip certain operations as needed. While not ideal, this is better than SSR failing and I don't think we're going to get a usable CSS solution anytime soon.

@e111077
Copy link

e111077 commented Aug 24, 2022

For slot selection, since we don't have a proper CSS solution right now, what if we ask users that want to support SSR with no FOUC to manually say whether or not the element has a slot etc. For example sl-select:

export default class SlSelect extends ShoelaceElement {
  ...
  private readonly hasSlotController: HasSlotController|null = null;
  ...
  @property({type:boolean, attribute: 'has-label'}) hasLabel = false;
  @property({type:boolean, attribute: 'has-help-text'}) hasHelpText = false;
  ...
  // runs only on client and before first render so no FOUC if not SSR
  update(changed: PropertyValues<this>) {
    if (!this.hasSlotController) {
      this.hasSlotController = new HasSlotController(this, 'help-text', 'label');
    }
    
    super.update(changed);
  }
  ...
  render() {
    // on server: uses the host property
    // on client: hasSlotController should be ready by render and update accordingly
    const hasLabelSlot = this.hasSlotController?.test?.('label') ?? this.hasLabel;
    const hasHelpTextSlot = this.hasSlotController?.test?.('help-text') ?? this.hasHelpText;
    ...
  }
  ...
}

SSR Users

This will allow shoelace to be SSRd, and once hydrated on the client, HasSlotController can kick in and make sure the render is correct and update styles. If the user wants to SSR and not have a FOUC flicker, then they can explicity say has-icon or has-help-text.

Normal People

If the user doesn't care about SSR, then their component should function exactly the same under the hood with negligible performance overhead from checking some if statements.

DX

DX can probably be cleaned up for the render method by making a util function. Update can probably be cleaned up in a mixin or in ShoelaceElement. The @propertys cannot really be cleaned up :(

The future

This can work as a workaround until we get a proper CSS solution for this. When we do get a proper CSS solution in enough browser coverage, you'd have to strike a breaking version change to remove the added properties and replace both this workaround + HasSlotController with the proper CSS solution

@claviska
Copy link
Member

claviska commented Aug 25, 2022

The @propertys cannot really be cleaned up :(

The footprint is my biggest concern, but creating a convention might reduce it and make it more mixin-friendly. What about something like this?

<sl-card ssr-slots="header footer image">

I can probably add ssr-slots to ShoelaceElement to make this a lot easier.

It would need to support default slots too, which are currently represented by [default] internally. It seems like this would be OK to carry over into the mixin:

<sl-card ssr-slots="[default] header footer image">

Does this seem like a reasonable solution? Are there any other things we should be considering in terms of SSR, or is slot detection the last big blocker?

@e111077
Copy link

e111077 commented Aug 25, 2022

I love the idea of using ssr- to denote "YOU ARE DOING THIS TO YOURSELF" but a little less of a fan of -slots because it sounds a bit leaky compared to has-icon. Regardless, I like it much better overall than my suggestion. What would it look like in the mixin?

// in the mixin
@property({attribute: 'ssr-slots', converter: {
  fromAttribute: val => {
    return val.split(' ');
  },
  toAttribute: val => {
    return val.join(' ');
  }
}})
ssrSlots = [];

protected readonly hasSlotController: HasSlotController|null = null;

update(changed: Map<string, unknown>) {
  this.hasSlotController = new HasSlotController(this, ...this.ssrSlots);
  super.update(changed);
}

protected slotTest(value: string) {
  return this.hasSlotController?.test?.(value) ?? this.ssrSlots.includes(value);
}

// in the actual code
export class Select extends SsrSlotMixin(SlElement) {
...
render() {
const hasLabelSlot = this.slotTest('label');
const hasDefaultSlot = this.slotTest('[default]');
...

In terms of other missing things I'm not totally sure because one error might be hiding another one. I'll take a look at seeing if I can set something sharable up in the coming days

@claviska
Copy link
Member

To update: all of the aforementioned issues have been resolved now except for slot detection.

What would be the best "standard" way to move forward with an alternative for slot detection? Before we do custom things, is there any emerging best practice for doing this in Lit or other libraries?

@justinfagnani
Copy link
Contributor

justinfagnani commented Sep 19, 2022

Awesome news!

For slot detection, I think we should investigate whether Lit can support something directly.

I don't think we'd want to support slot.assignedElement, because we'd need to support a way to get a HTMLSlotElement, likely via a query. This would be an uncanny valley of DOM support.

But we might be able to add SSR-specific code to something like the SlotController RFC so that it's hasAssignedElements() method works. Even that should be used judiciously, since detecting whether an element is slotted requires parsing HTML, which we avoid in many cases.

We also have to consider rendering order to continue to support streaming. Reading slot contents requires rendering children, which usually wouldn't have happened until after the host's shadow root is rendered, so we'd have to buffer the children to stream out later.

cc @aomarks @augustjk @sorvell and @kevinpschaaf from the Lit team.

@claviska
Copy link
Member

Two strategies are currently available to work around imperative slot detection:

User-provided Attributes

The user provides attributes indicating which slots have been populated. This is extra work for end users, as they have to slot the content and add an attribute.

If we decide to to this as a stopgap, I'd prefer to use a single attribute instead of littering the API with multiple properties. Perhaps something like this.

<sl-button variant="default" has-slots="prefix suffix">
  <sl-icon slot="prefix" name="link-45deg"></sl-icon>
  <sl-icon slot="suffix" name="box-arrow-up-right"></sl-icon>
  Open
</sl-button>

I'd also prefer to keep the has-slots attribute completely optional, so only users that care about SSR will need to provide them. Eventually, once we have a CSS selector like :has-slot(), we can easily deprecate this without affecting users.

Collapsible Containers

Instead of programmatically showing/hiding containers based on the presence of a slot, we bear the burden of removing the slot detection controller in favor of containers and styles that effectively collapse the container when no content is slotted.

This might not be as easy as it sounds for all existing components, but it's worth exploring.

@e111077
Copy link

e111077 commented Aug 17, 2023

presented update proposal to WCCG for TPAC prep. Here is my current draft:

w3c/webcomponents-cg#72

TL;DR, I think it would be good to help advance the /slotted/ combinator over :has-slotted and people at WCCG meeting agreed but we need to send feelers out to browser implementers.

w3c/csswg-drafts#7922

we would be able to do things like so:

.label:has(~ .icon /slotted/ *) {
   /* label icons when only icon sibling has slotted nodes */
}

please leave comments on that discussion for what you would like for me to add to the status update to TPAC browser implementers.

UPDATE: Browser implementers at TPAC very much disliked /slotted/. We will try to go again with ::has-slotted()

@claviska
Copy link
Member

TL;DR, I think it would be good to help advance the /slotted/ combinator over :has-slotted and people at WCCG meeting agreed but we need to send feelers out to browser implementers.

I'm out of the loop, but why do they prefer a new syntax over a more common pseudo selector? It seems like things will get confusing, especially since we already have ::slotted() and now /slotted/.

@e111077
Copy link

e111077 commented Aug 17, 2023

it's honestly an ambitious proposal, but /selector/ is supposedly the future of new selectors according to the CSSWG since they are running out of special characters. We have historically had the /deep/ shadow dom piercing selector in web components V0 which was shipped on chrome at one point.

But the benefits are that it solves a lot of issues that ::slotted() has simply because ::slotted() a pseudo element:

  • it would allow query selectors shadowRoot.querySelector('.icon /slotted/ *')
  • it could allow :has(/slotted/) selectors
  • it would allow sibling /slotted/ + * selectors
  • it would allow selecting pseudo elements /slotted/ dialog::backdrop
  • WK implementers floated that it might be able to select more than just the top-level slotted element but personally IDK if that will actually float

It gives a unified approach to shadow dom selectors that are more inline with selectors outside of shadow DOM (one less shadow dom smell and not "just-another-shadow-dom-thing") and it fixes more issues around slotting.

The "what would happen to ::slotted() question is still open, but we are at the stage where we need to get feelers from browser vendors before we tackle bigger questions like that + how do we handle flatten: true|false and other questions specific to the syntax, but those are the reasons that sorta pushed us in the direction of the /slotted/ combinator over the more-specific :has-slotted()

@claviska
Copy link
Member

Ahh, thanks for the explanation. I wasn't aware of the new syntax and assumed it was made for this purpose alone. I like everything I'm hearing, though. And it makes sense that ::slotted() may have an uncertain future. I would support it being deprecated if /slotted/ succeeds.

Thanks again!

@jaredcwhite
Copy link
Contributor

@e111077 That's a great explanation, thank you.

@reggi
Copy link

reggi commented Oct 8, 2023

Any updates on what's holding up SSR for Shoelace? I tried building in Astro and ran into a bunch of DOM stuff MutationObserver, couple document, and window calls. I'd love for this to work 🤞.

@shoelace-style shoelace-style locked and limited conversation to collaborators Oct 16, 2023
@claviska claviska converted this issue into discussion #1641 Oct 16, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
feature Feature requests. help wanted Ready for a contributor to tackle.
Projects
None yet
Development

No branches or pull requests

7 participants