solidjs / solid-start

SolidStart, the Solid app framework
https://start.solidjs.com
MIT License
5.19k stars 375 forks source link

[Bug?]: client only doesn't work anymore for `props.children` #1427

Closed BierDav closed 7 months ago

BierDav commented 7 months ago

Duplicates

Latest version

Current behavior 😯

Recently I updated from an old solidjs version to the latest also the new router. And noticed that the clientOnly method which was experimental back than doesn't work when wrapping props.children. This is my code:

Helper:

import { clientOnly } from "@solidjs/start";
import { Component } from "solid-js";

export default function clientOnlyComponent<T>(component: Component<T>) {
  return clientOnly(async () => ({ default: component }));
}

Usage:

import { useAuthSession } from "~/api";
import { Typography } from "@suid/material";
import TextMediaWrapper from "~/ui/TextMediaWrapper";
import { ParentProps, Show, Suspense } from "solid-js";
import clientOnlyComponent from "~/lib/util/clientOnlyComponent.ts";

export default function (props: ParentProps) {
  const [authSession] = useAuthSession();
  return (
    <>
      <Suspense>
        <Show
          when={authSession()}
          fallback={
            <TextMediaWrapper>
              <Typography variant={"h4"} sx={{ textAlign: "center" }}>
                Access Denied
              </Typography>
            </TextMediaWrapper>
          }
        >
          {authSession()?.isExpired
            ? clientOnlyComponent(() => props.children)({})
            : props.children}
        </Show>
      </Suspense>
    </>
  );
}

With the old <Outlet/> router, everything worked fine, but unfortunatey this implementation causes an endless loop on the browser after hydration which makes it unusable.

Expected behavior 🤔

A way to make props.children client only that doesn't cause and endless loop in the browser which completely freezes the tab.

Steps to reproduce 🕹

Steps:

  1. Copy my code
  2. Paste my code
  3. Fix syntax errors
  4. Run it ;)

You may experience it yourself in this neight little sample project. But don't be supprised that your tab freezes and you might have to force close it. I warned you 😉 https://stackblitz.com/~/github.com/BierDav/solid-start-ssr-bug

Context 🔦

I need the client only, because when the token that is stored in the request cookie is expired only the client can refresh it, and it wouldn't make sense to wait for all the unauthorized responses I would get when evaluating the ssr.

Your environment 🌎

System:
 OS: Windows

Node: v21.7.1

Packages:
 vinxi: ^0.3.10
 solid-js: ^1.8.15
 @solidjs/router: ^0.13.1
 @solidjs/start: ^0.7.7
lxsmnsyc commented 7 months ago

There's some bad patterns here, but mainly I'd just recommend using this component:

import { createSignal, onMount, Show } from 'solid-js';

function ClientOnly(props) {
  const [flag, setFlag] = createSignal(false);

  onMount(() => setFlag(true));

  return <Show when={flag()}>{props.children}</Show>;
}

(or just install solid-use and use solid-use/client-only)

But to explain why your usage is wrong:

  1. Client-only is meant to be used with a dynamic import. Without the dynamic import, you are still loading the component's module on the server-side.
  2. Your ternary condition still renders the props.children without guaranteeing that it renders only on the client-side.
  3. Your client-only factory would be the reason why it's triggering an infinite loop. It's just a pattern that you should not do.
BierDav commented 7 months ago

@lxsmnsyc Thanks for your help makes sense, maybe we should add something like a ClientOnlyBoundary in future, to make this clear. Unfortunately the current solid start documenation is quite bad when it comes to advanced requirements.

lxsmnsyc commented 7 months ago

The reason clientOnly was implemented first was because of the fact that there are libraries that provides a client-only side-effect, which triggers errors when imported on the server. clientOnly forces the user to import these dynamically to prevent the modules from being loaded immediately.

BierDav commented 7 months ago

Ok, I see, but that doesn't mean, that a ClientOnlyBoundary component wouldn't be useful. I think that such small util functions that directly work together with the solid start engine (I would call it) should be provided by solid-start for convenience. And this would be a nice addition.

ryansolid commented 7 months ago

Understanding this issue, I think this is no action. As @lxsmnsyc responded clientOnly is for dynamic imports. ClientBoundary is interesting but it is also another thing. When it is an official component we have to explain when to use one over the other. It's one of those cases where while it could be made into another component, it could be done more specifically for the situation in a number of ways and it is minimal code. I see solid-use has a prebuilt of it so it is readily available. Which makes me more inclined to defer any action on this until it is clear where the responsibility for this should lie.

BierDav commented 7 months ago

Makes sense, thanks for your detailed answer