Render async SolidJS components #6791
Conversation
🦋 Changeset detectedLatest commit: 93fecc7 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
pkg: astro
|
I'm going to tag @Jutanium here as the Docs Maintainer to decide whether this would require an update to the Solid integration README if accepted. |
|
I would want to edit the SolidJS Integration Readme if this gets accepted |
|
Thank you for the initial feedback. I tried to address the comment about how to track page context. In the process I noticed a new bug in the implementation where it is still generating "hydration events/patches" even though |
|
I found the source of the error, it turns out there is a bug in how NoHydration works in SolidJS v1.5.6, the version used in the Astro repository. After updating to v1.6.0, the tests passed. In version 1.5.6, with the following input: function SyncErrorComponent() {
throw new Error('Sync error thrown!');
}
function SyncErrorInErrorBoundary() {
return (
<ErrorBoundary fallback={<div>Sync error boundary fallback</div>}>
<SyncErrorComponent />
</ErrorBoundary>
);
}
const App = () => {
return (
<NoHydration>
<Suspense>
<SyncErrorInErrorBoundary />
</Suspense>
</NoHydration>
);
};
const out = await renderToStringAsync(() => App);SolidJS would render the following HTML on the server: <div>Sync error boundary fallback</div>
<script>
_$HY.set(
"0-0",
Object.assign(new Error("Sync error thrown!"), {
stack:
"Error: Sync error thrown!\n at SyncErrorComponent (file:///home/pm/dev/solid-ssr-error-test/out.js:13:9)\n at createComponent (file:///home/pm/dev/solid-ssr-error-test/node_modules/.pnpm/[email protected]/node_modules/solid-js/dist/server.js:283:10)\n at get children [as children] (file:///home/pm/dev/solid-ssr-error-test/out.js:21:14)\n at file:///home/pm/dev/solid-ssr-error-test/node_modules/.pnpm/[email protected]/node_modules/solid-js/dist/server.js:372:24\n at createMemo (file:///home/pm/dev/solid-ssr-error-test/node_modules/.pnpm/[email protected]/node_modules/solid-js/dist/server.js:73:9)\n at ErrorBoundary (file:///home/pm/dev/solid-ssr-error-test/node_modules/.pnpm/[email protected]/node_modules/solid-js/dist/server.js:368:3)\n at createComponent (file:///home/pm/dev/solid-ssr-error-test/node_modules/.pnpm/[email protected]/node_modules/solid-js/dist/server.js:283:10)\n at SyncErrorInErrorBoundary (file:///home/pm/dev/solid-ssr-error-test/out.js:16:10)\n at createComponent (file:///home/pm/dev/solid-ssr-error-test/node_modules/.pnpm/[email protected]/node_modules/solid-js/dist/server.js:283:10)\n at get children [as children] (file:///home/pm/dev/solid-ssr-error-test/out.js:30:18)",
})
);
</script>This feels incorrect because the Error and ErrorBoundary are wrapped inside a NoHydration context, so there shouldn't be any reason to serialize the ErrorBoundary state. In Solid 1.6.0 (and in latest version 1.7.3), the same input code renders the following HTML on the server: <div>Sync error boundary fallback</div>Actually I can reproduce a similar error with plain renderToString or even with Solid-js v1.5.6 in newly created Astro project too, so it may be wise to upgrade to SolidJS 1.6.0 or higher regardless of this PR. I updated all the SolidJS version references in the repo to "^1.6.0", and added a line mentioning this to the changeset too, though I wonder if there is a preferred way to handle a peer dependency minimum version change? |
|
Would fix #6912 |
|
Awesome @patdx can you update to the latest Solid version in that case? |
|
Hi @matthewp I updated all the references to the latest SolidJS version, 1.7.4, at the time of writing. I also added a test based on the #6912 reproduction example. #6912 lists three possible outcomes:
As far as I can tell (1) seems to be resolved no matter how times I refresh in dev or preview mode. Most of the time I got the successful result (3), however I could still still observe some issue like (2) that randomly occurred when refreshing. So I believe this PR may improve the behavior of #6912 but not fix it entirely. |
There was a problem hiding this comment.
Just blocking this one for docs so it doesn't slip past me. If accepted, this has potential breaking changes for existing users and will require an update to the Solid integration README to document this behaviour.
@patdx - When we are confident in this being accepted, and the user-facing behaviour is stable enough to know what we need to explain to readers (behind the scenes implementation details don't need to be documented, so it doesn't matter to docs how you get this behaviour working! 😄), please take an initial stab at documenting the new behaviour in the README (finding the logical place, adding the behaviour) and @Yan-Thomas and I will review!
github-actions
bot
removed
the
pkg: example
|
I made a first attempt at updating the SolidJS readme: https://github.com/withastro/astro/pull/6791/files#diff-7612e1e1749634879a227c7e1362b864b1648cf6ca4d6b06968bf995e3f1f7ca The tests seem to be passing, so I feel like it accomplishes the original goal of supporting async rendering. For #6912, I believe the async rendering implemented by this PR fixes the SSR for lazy() to work more consistently. Based on my own super quick test, it did seem like maybe there was another timing/race condition thing that still occasionally messed up hydration. I am not sure if that is an issue is on the Solid or Astro side. I'm also not really sure how to start debugging it. Personally I think it may be easier to debug or escalate any remaining issues if async rendering is implemented first. But if we want to make sure that #6912 is 100% resolved before possibly merging this PR, I think I could use some extra help testing/debugging that as I'm not really sure what to do next. |
|
I made a super quick attempt to rebase the PR with the latest version. Depending on the implementation of the new parallelized rendering, it may be necessary adjust the Server render logic. If there are hydratable Solid components on the page, the official SolidJS hydration script needs to be inserted as an inline script on the page before the first hydratable Solid component in the HTML. Also, I didn't realize but I guess it is possible to have Astro Solid islands rendered inside of Astro Solid islands? <!-- index.astro -->
<SolidA>
<SolidB />
</SolidA>
<SolidC/>If SolidB may be rendered before SolidA then I think I can't just rely on "which render function was called first", need to use extraHead or something to inject the hydration script before the first component on the page. |
|
Hey @patdx thanks for tackling this! Sorry things have been quiet! Are you still interested in working on this? If so, we'd be happy to help get this back on track. |
Hi @natemoo-re sorry yes I am still interested in working on this, but I only have a few hours per week max to work on this kind of stuff, and I ran out of energy to keep up recently, I guess I am a little re-energized now. But still, if somebody is interested to take over I am happy to step back :) I tried to rebase and resolve my own open question about nested components. I did some testing and found it actually worked mostly correctly by default. But I was able to come up with a pretty contrived situation where the inner SolidJS component was rendered first, which caused the SolidJS hydration script to be rendered in the wrong spot (or not at all). So I moved the SolidJS hydration script to |
|
I updated the readme to clarify that that Suspense is only automatically wrapped around hydrating components, not non-hydrating components ( I set it up this way because I feel like it is a good default to make the renderToStringAsync on the server wait for However, if we automatically wrapped Suspense around a client:only component that used |
|
Hey @patdx wanted to bump this tread because I'd love to see this get merged in. Also, follow-up questions but what would be required to get this to work with solid's streaming. Astro already streams its page down so would that even be possible or would that break Astro's current component model? |
@howagain I did a quick rebase, hmm I think the PR is basically feature complete, with all the tests I can think of. I need help from the Astro team to review the technical strategy and documentation when they have some time.
Hmm, I'm not sure how that would work. I don't understand the full rendering story, but I think that Astro expects the Renderers (React, Solid, etc) to provide a function called rendertoStaticMarkup that returns There would still be some other issues to overcome: (1) I'm not sure how you would support streaming with components nested inside components, especially if different frameworks. Although many frameworks support streaming output, I don't think many (or any?) frameworks support streaming arbitrary HTML inside of them so nesting components may deopt streaming. (2) It seems possible in Astro for a nested component to start "rendering" before its parent starts rendering. This is a problem because SolidJS also needs a Hydration Script injected in the HTML before the first top-level SolidJS component, and we can't rely on the rendering order nor any other method (that I can think of) to detect which rendering component is the first top-level component. I solved this problem in this PR using extraHead to make sure the Hydration Script got injected before the first SolidJS component regardless of rendering order. However, if Astro started streaming everything, I think we would have to assume that the |
|
@sarah11918 what all is required to get this merged in? I'd love to start taking advantage of solid's |
|
Hi @howagain! I know it's my block you're seeing, but that's because this PR would cause breaking changes and we want to make sure nothing goes out without appropriate guidance in documentation. Normally, when a feature is approved and the implementation is confirmed by the dev team, then the docs team would be pinged to oversee documentation. (Not every PR we receive is accepted and approved.) The maintainer who would normally oversee this documentation is no longer with the project, but I'll put this one on @ElianCodes ' radar. We would look at it once @natemoo-re or @matthewp tells us it's likely to be approved! So, I guess docs is the bottleneck, but at the same time is not the bottleneck here. 😄 |
github-actions
bot
added
the
pr: docs
Thank you @johannesspohr. I liked your solution so I cherry picked your commit into this PR, hope that is okay. I do wonder if it will be acceptable to modify the core like this. Let's see if there is any additional feedback about this approach from the core team.
Hi @sarah11918 it's pretty complete. Even if there are more changes after technical review I don't think they affect the docs very much. I took another look and can't think what else to say, hopefully it is on the right track. Would greatly appreciate some feedback on the documentation. Thank you! |
|
Thanks, just a heads up this is on my list for this week (reviewing the docs!) |
There was a problem hiding this comment.
Hi @patdx ! Thanks for your patience with docs! This looks like an amazing feature, and I'm sure it will be a welcome update to the Solid integration!
The changeset was really nicely written! I appreciate your attention to breaking changes, and I felt like there was good context provided for a major upgrade! 🙌
I made some suggestions re: tackling this topic in the README. I think my main point is self-explanatory, (it's the same for both examples) but feel free to ask if you have any questions about it!
|
(Sorry, I accidentally posted from the wrong account, so I have deleted and am reposting the same message) I made an update to address the documentation feedback. I also changed the version of Solid JS everywhere to 1.8.5 This is for two reasons:
|
|
What's the status of this PR apart from the conflicts? AFAIK docs is fine, what about implementation itself? |
|
@florian-lefebvre The code is completed and passing the tests. I think the next step is to get a technical review from Astro core team. I'm not sure who would be in charge. Maybe @natemoo-re or @matthewp could help take a look? While the code is complete to the best of my knowledge, we ended up introducing a new I just resolved the conflicts again. Looks like the Solid JS version got bumped to 1.8.5 in main branch of Astro last week, which makes this PR smaller :) |
packages/astro/src/@types/astro.ts
Outdated
| @@ -2480,6 +2485,10 @@ export interface SSRResult { | |||
| */ | |||
| export interface SSRMetadata { | |||
| hasHydrationScript: boolean; | |||
| /** | |||
| * Keep track of which renderers already injected their specific hydration script | |||
There was a problem hiding this comment.
It's worth explaining what we save in this record, especially what's inside the key of the record.
Also, the name hints at a boolean value, but it's a map. Maybe just hydrationScripts is enough?
There was a problem hiding this comment.
Noted. I am editing the comment. Also, I'm going to change it to a Set<string> as this matches the pattern of hasDirectives and propagators which might make it easier to understand.
The naming is tricky. The name hydrationScripts could almost work, however it may be too confusing next to the unrelated hasHydrationScript. I think that's why @johannesspohr, who helped write this part of the PR, originally added RendererSpecific, to make it more clearly different from hasHydrationScript.
Thinking of it as a Set, I'm planning to revise it slightly to rendererSpecificHydrationScripts. Then I can rewrite the code in packages/astro/src/runtime/server/render/common.ts in a slightly easier to read way:
const { rendererSpecificHydrationScripts } = result._metadata;
const { rendererName } = instruction;
if (!rendererSpecificHydrationScripts.has(rendererName)) {
rendererSpecificHydrationScripts.add(rendererName);
return instruction.render();
}
return '';
packages/astro/src/@types/astro.ts
Outdated
| * If set, allowes the renderer to print a specific hydration script before | ||
| * the first hydrated island |
There was a problem hiding this comment.
I believe this option requires a more descriptive explanation. It's OK if you know what this function does, but unfortunately I don't understand its use case, so it's worth explaining at least why this is needed for an island.
| rendererName: renderer.name, | ||
| render: renderer?.ssr.renderHydrationScript, |
There was a problem hiding this comment.
I don't think the optional chaining operator ? is needed, because render is a mandatory property
| * framework | ||
| */ | ||
| export type RendererHydrationInstruction = { | ||
| type: 'renderer-hydration'; |
There was a problem hiding this comment.
Should we call it hydration-script? At the end, that's the actual meaning of this instruction
There was a problem hiding this comment.
You're right, the thing it's actually doing is rendering a hydration script, not doing the actual hydration. (Hydration happens after.) However, I feel that hydration-script would be confusingly similar to the unrelated hasHydrationScript thing. For the benefit of future developers I think it would be good to keep these clearly separated. I will rename it to renderer-hydration-script for now. Please take a look again after I push the next commit and let me know what you think?
| @@ -89,6 +89,13 @@ function stringifyChunk( | |||
| } | |||
| return renderAllHeadContent(result); | |||
| } | |||
| case 'renderer-hydration': { | |||
| if (!result._metadata.hasRendererSpecificHydrationScript[instruction.rendererName]) { | |||
There was a problem hiding this comment.
I don't understand this check, why do we check against false values? Shouldn't we remove the if block?
There was a problem hiding this comment.
The Solid hydration script only needs to be rendered once per page, so this is a check to make sure it has not been rendered yet. If we removed the if, it would get rendered once per component instead, which would probably still work, but would not be ideal as it would be sending some redundant data to the client.
| // Prepare global object expected by Solid's hydration logic | ||
| if (!(window as any)._$HY) { | ||
| (window as any)._$HY = { events: [], completed: new WeakSet(), r: {} }; | ||
| } |
There was a problem hiding this comment.
Did we remove this because we don't need it anymore?
There was a problem hiding this comment.
This code has been moved to the renderHydrationScript directive thing and now using the official hydration script generated by Solid's generateHydrationScript() function.
Background: The hydration script creates a global _$HY object. During Solid's SSR phase, it may serialize data and inject it into inline scripts on the page that depend on the existence of the _$HY object. These hydration event scripts look similar to the snippet below:
<script>(self.$R=self.$R||{})["s0"]=[]_$HY.r["s00-2-0"]="$$f"`</script>Hydration events are uncommon in sync SSR, but very common in async SSR (especially when using createResource, Suspense or ErrorBoundary). This file packages/integrations/solid/src/client.ts loads too late to handle any actual hydration event scripts. Therefore it becomes necessary to inject the hydration script into HTML directly, before any of the hydration event scripts.
| // a hyphen at the end makes it easier to distinguish the island | ||
| // render id from component tree portion of the hydration key `data-hk="..."` | ||
| // https://github.com/solidjs/solid-start/blob/f0860887030e0632949b3f497e279aecb6ed5afd/packages/start/islands/mount.tsx#L41 |
There was a problem hiding this comment.
This heuristic seems to be very brittle. I wonder if there was a more "solid" way to "signal" this (sorry, the puns weren't intended 😆 )
There was a problem hiding this comment.
I'll just remove it from the PR because I don't want the PR to get distracted by this. I changed this because it made debugging hydration errors a little bit easier and forgot about it, but now that you mention it, I realize that technically it's not related or essential. Maybe me or someone else can make a PR for this some other time. :)
|
I rebased the PR and added a new commit to address @ematipico's feedback. Thanks! |
| @@ -98,6 +98,41 @@ export default defineConfig({ | |||
| }); | |||
| ``` | |||
|
|
|||
| ## Async rendering strategy | |||
There was a problem hiding this comment.
@patdx Making a note that this change should now be PR'd to the docs repo directly!
We have updated all the integration READMEs to a minimal format, so instead, please make a PR in the docs repo for this content change at: https://github.com/withastro/docs/blob/main/src/content/docs/en/guides/integrations-guide/solid-js.mdx and do not edit the Solid-js README. Thank you!
There was a problem hiding this comment.
Got it, I made a PR for the docs here: withastro/docs#6027
There was a problem hiding this comment.
Thanks, got it! We'll pick this part of the conversation up over there! 🥳
…-to-string-async-solid
|
I merged and resolved conflicts in the last commit, including reverting the readme changes. I created a separate PR for the Solid docs change here: withastro/docs#6027 |
Changes
@johannesspohr also contributed code and tests to this PR.
Testing
I designed some test scenarios and wrote some tests conforming the existing/nonexistence of hydration scripts depending on the client directive.
Docs
This PR changes how server-only and hydrating SolidJS components may resolve async resources. Before, they may have resolved them on the client side. Now, they may be resolved on the server side before rendering. I tried to cover the changes in the changeset and the SolidJS integration readme.
/cc @withastro/maintainers-docs for feedback!