SolidJS•2y ago

Lazy in For results in renderToString timed out in production build

Using a bare solid-start project with the following code, and running the prod build of it, results in a "renderToString timed out". To be precise: the first request results in this time out. Every further request works without timeout. Node version: v16.13.0

import { For, lazy } from "solid-js";

const LazyCounter = lazy(async() => import('~/components/Counter'))

export default function Home() {
  return (
    <main>
      <For each={[1,2,3]}>
        {() => {
          return <LazyCounter />
        }}
      </For>
    </main>
  );
}

import { For, lazy } from "solid-js";

const LazyCounter = lazy(async() => import('~/components/Counter'))

export default function Home() {
  return (
    <main>
      <For each={[1,2,3]}>
        {() => {
          return <LazyCounter />
        }}
      </For>
    </main>
  );
}

19 Replies

Katja (katywings)OP•2y ago

@ryansolid I figured out from were the bug is coming 🤓 . The problem lies in this line: https://github.com/solidjs/solid/blob/bd5b5dbdc4db35a71787570b93097035535dfe93/packages/solid/src/server/rendering.ts#L501

GitHub

solid/rendering.ts at bd5b5dbdc4db35a71787570b93097035535dfe93 · so...

A declarative, efficient, and flexible JavaScript library for building user interfaces. - solid/rendering.ts at bd5b5dbdc4db35a71787570b93097035535dfe93 · solidjs/solid

Katja (katywings)OP•2y ago

notifySuspense clears the contexts even if not all are completed if a lazy component is rendered in a For it will create a context for every For item When the first item finishes loading it calls notifySuspense which currently clears all contexts c.completed() will never be called for all contexts which were removed from the contexts too early I think contexts.clear(); should only be called when all contexts are completed Let me know if this is the proper logic, then i can implement a bugfix for this 🙂

ryansolid•2y ago

Hmm... I think I was expecting that whole function to only be called once at the end but I think you are right, multiple async in flights kinda breaks it.

Katja (katywings)OP•2y ago

I think something like this could be a solution:

for (const c of contexts) {
  if (!suspenseComplete(c)) {
    continue;
  }
  
  c.completed();
  contexts.delete(c)
}

// or if you like it short:
for (const c of contexts) {
  if (suspenseComplete(c)) c.completed() && contexts.delete(c);
}

for (const c of contexts) {
  if (!suspenseComplete(c)) {
    continue;
  }
  
  c.completed();
  contexts.delete(c)
}

// or if you like it short:
for (const c of contexts) {
  if (suspenseComplete(c)) c.completed() && contexts.delete(c);
}

But I need to try it out when I am back on my dev pc 🙂

ryansolid•2y ago

Yeah that is probably fine. But I'm more wondering if this miss has other side effects.

Katja (katywings)OP•2y ago

You mean the fix might change how the lazy works on other places and maybe people expect it to work, as it currently does? Or something else? @ryansolid The tests (pnpm test) still succeed after the fix, but this probably doesnt mean a lot since there are no existing tests specifically for lazy 🙈 How do you wanna proceed with this? Are you interested in a PR from me, or not? 🤓 (p.s. i tried to create a new test specifically for this fix, but couldn't figure out how to render lazy without a full ssr setup 🙈). Something like this didn't work as a test 😅

createRoot(() => {
  const Comp = lazy(async () => ({ default: () => 1 } ))
  const out = createComponent(Suspense, {
    children: () => ([
      createComponent(ErrorBoundary, {
        children: () => ([
          createComponent(Comp, {})
        ])
      })
    ])
  });
  expect(out()).toBe("...something...");
});

createRoot(() => {
  const Comp = lazy(async () => ({ default: () => 1 } ))
  const out = createComponent(Suspense, {
    children: () => ([
      createComponent(ErrorBoundary, {
        children: () => ([
          createComponent(Comp, {})
        ])
      })
    ])
  });
  expect(out()).toBe("...something...");
});

ryansolid•2y ago

Yeah async function forces it to wait so timing might be important .. like might need to hoist the out outside of the root and then wait a tick so that lazy resolves Not about the fix. More just my bad assumption in the first place that would allow this issue could have other side effects I haven't consider. I think you can go ahead with the PR.

Katja (katywings)OP•2y ago

Ty! I'll try that out with the hoist and tick tomorrow 🙏

Katja (katywings)OP•2y ago

...this issue could have other side effects I haven't consider.

Ohhh check 👍. Personally I find it interesting, that the bug didnt happen earlier, like what does that tell about how people are using lazy? Looks like rendering a lazy component in a loop just is not a typical use case. Regarding issue side effects: since the bug shows itself very upfront with that time out, and the timeout went unnoticed for so long, i kinda assume, that the issue likely doesnt have side effects 🤔, but thats just theory making... The big commit that added the critical contexts.clear() line was: https://github.com/solidjs/solid/commit/5831f7a468e2e84de9f1f11340d6d53c21003fae - could it be that you maybe had planned to add more logic to notifySuspense and just forgot to rework that clear? Anyhow: i'll happily create a PR then 🙂.

GitHub

Update SSR, Events, Spreads, New Resource API, Children Helper, and...

…ore (#328) * New Async SSR WIP * update packages * v0.24.0-beta.0 * fix non-loading resources, new SSR API, jsx publish convention * v0.24.0-beta.1 * fix #322 state spreads * re...

Katja (katywings)OP•2y ago

@ryansolid I opened the PR here: https://github.com/solidjs/solid/pull/1430 Thank you for looking at it in advance 🤗 !

GitHub

fix: delete lazy contexts one by one as they are completed by katyw...

Summary When one lazy component is rendered multiple times in parallel, suspense will wait until every load is finished. The fixed bug was, that the first notifySuspense call cleared contexts of st...

ryansolid•2y ago

Hey @katywings is it just renderToString timing out, or also like renderToStringAsync.. I just noticed it was renderToString which is not async rendering, so we usually try to preload lazy or not lazy in dev for it cause it forces everything into suspense.

Katja (katywings)OP•2y ago

Mhhhh 🤔 , the timeout is happening on a bare solid-start project with the default entry-server.tsx that uses renderAsync

ryansolid•2y ago

ok .. I wonder why it says renderToString.. Anyway.. yeah ok we're good. Thanks for the PR.

Katja (katywings)OP•2y ago

Yeees, i also noticed it 🤔 I know it! Just figured it out xD

ryansolid•2y ago

What?

Katja (katywings)OP•2y ago

The error message in renderToStringAsync literally is "renderToString timed out"

ryansolid•2y ago

Hah.. ok.. so in DOM expressions.. Alright

Katja (katywings)OP•2y ago

https://github.com/ryansolid/dom-expressions/blob/9d4385463ffa8719b3f22e7962e53fd7f1d30e08/packages/dom-expressions/src/server.js#L34

GitHub

dom-expressions/server.js at 9d4385463ffa8719b3f22e7962e53fd7f1d30e...

A Fine-Grained Runtime for Performant DOM Rendering - dom-expressions/server.js at 9d4385463ffa8719b3f22e7962e53fd7f1d30e08 · ryansolid/dom-expressions

Katja (katywings)OP•2y ago

@ryansolid Thank your for the merge ♥️✌️💪

Gaming

Programming

Lazy in For results in renderToString timed out in production build