Skip to content
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

Render async SolidJS components #6791

Merged
merged 8 commits into from
Jan 4, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions .changeset/chilly-badgers-push.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
---
'@astrojs/solid-js': major
---

Render SolidJS components using [`renderToStringAsync`](https://www.solidjs.com/docs/latest#rendertostringasync).

This changes the renderer of SolidJS components from `renderToString` to `renderToStringAsync`. It also injects the actual SolidJS hydration script generated by [`generateHydrationScript`](https://www.solidjs.com/guides/server#hydration-script), so that [`Suspense`](https://www.solidjs.com/docs/latest#suspense), [`ErrorBoundary`](https://www.solidjs.com/docs/latest#errorboundary) and similar components can be hydrated correctly.

The server render phase will now wait for Suspense boundaries to resolve instead of always rendering the Suspense fallback.

If you use the APIs [`createResource`](https://www.solidjs.com/docs/latest#createresource) or [`lazy`](https://www.solidjs.com/docs/latest#lazy), their functionalities will now be executed on the server side, not just the client side.

This increases the flexibility of the SolidJS integration. Server-side components can now safely fetch remote data, call async Astro server functions like `getImage()` or load other components dynamically. Even server-only components that do not hydrate in the browser will benefit.

It is very unlikely that a server-only component would have used the Suspense feature until now, so this should not be a breaking change for server-only components.

This could be a breaking change for components that meet the following conditions:

- The component uses Suspense APIs like `Suspense`, `lazy` or `createResource`, and
- The component is mounted using a *hydrating* directive:
- `client:load`
- `client:idle`
- `client:visible`
- `client:media`

These components will now first try to resolve the Suspense boundaries on the server side instead of the client side.

If you do not want Suspense boundaries to be resolved on the server (for example, if you are using createResource to do an HTTP fetch that relies on a browser-side cookie), you may consider:

- changing the template directive to `client:only` to skip server side rendering completely
- use APIs like [isServer](https://www.solidjs.com/docs/latest/api#isserver) or `onMount()` to detect server mode and render a server fallback without using Suspense.
13 changes: 11 additions & 2 deletions packages/astro/e2e/shared-component-tests.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { expect } from '@playwright/test';
import { scrollToElement, testFactory, waitForHydrate } from './test-utils.js';

export function prepareTestFactory(opts) {
export function prepareTestFactory(opts, { canReplayClicks = false } = {}) {
const test = testFactory(opts);

let devServer;
Expand Down Expand Up @@ -104,7 +104,16 @@ export function prepareTestFactory(opts) {
await waitForHydrate(page, counter);

await inc.click();
await expect(count, 'count incremented by 1').toHaveText('1');

if (canReplayClicks) {
// SolidJS has a hydration script that automatically captures
// and replays click and input events on Hydration:
// https://www.solidjs.com/docs/latest#hydrationscript
// so in total there are two click events.
await expect(count, 'count incremented by 2').toHaveText('2');
} else {
await expect(count, 'count incremented by 1').toHaveText('1');
}
});

test('client:only', async ({ page, astro }) => {
Expand Down
7 changes: 6 additions & 1 deletion packages/astro/e2e/solid-component.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { prepareTestFactory } from './shared-component-tests.js';

const { test, createTests } = prepareTestFactory({ root: './fixtures/solid-component/' });
const { test, createTests } = prepareTestFactory(
{ root: './fixtures/solid-component/' },
{
canReplayClicks: true,
}
);

const config = {
componentFilePath: './src/components/SolidComponent.jsx',
Expand Down
9 changes: 9 additions & 0 deletions packages/astro/src/@types/astro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2261,6 +2261,11 @@ export interface SSRLoadedRenderer extends AstroRenderer {
attrs?: Record<string, string>;
}>;
supportsAstroStaticSlot?: boolean;
/**
* If set, allowes the renderer to print a specific hydration script before
* the first hydrated island
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
renderHydrationScript?: () => string;
};
}

Expand Down Expand Up @@ -2480,6 +2485,10 @@ export interface SSRResult {
*/
export interface SSRMetadata {
hasHydrationScript: boolean;
/**
* Keep track of which renderers already injected their specific hydration script
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@patdx patdx Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 '';

*/
hasRendererSpecificHydrationScript: Record<string, boolean>;
hasDirectives: Set<string>;
hasRenderedHead: boolean;
headInTree: boolean;
Expand Down
1 change: 1 addition & 0 deletions packages/astro/src/core/render/result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ export function createResult(args: CreateResultArgs): SSRResult {
headInTree: false,
extraHead: [],
propagators: new Set(),
hasRendererSpecificHydrationScript: {},
},
};

Expand Down
7 changes: 7 additions & 0 deletions packages/astro/src/runtime/server/render/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@ function stringifyChunk(
}
return renderAllHeadContent(result);
}
case 'renderer-hydration': {
if (!result._metadata.hasRendererSpecificHydrationScript[instruction.rendererName]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this check, why do we check against false values? Shouldn't we remove the if block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result._metadata.hasRendererSpecificHydrationScript[instruction.rendererName] = true;
return instruction.render();
}
return '';
}
default: {
throw new Error(`Unknown chunk type: ${(chunk as any).type}`);
}
Expand Down
9 changes: 9 additions & 0 deletions packages/astro/src/runtime/server/render/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,15 @@ If you're still stuck, please open an issue on GitHub or join us at https://astr
}
}
destination.write(createRenderInstruction({ type: 'directive', hydration }));
if (hydration.directive !== 'only' && renderer?.ssr.renderHydrationScript) {
destination.write(
createRenderInstruction({
type: 'renderer-hydration',
rendererName: renderer.name,
render: renderer?.ssr.renderHydrationScript,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the optional chaining operator ? is needed, because render is a mandatory property

})
);
}
destination.write(markHTMLString(renderElement('astro-island', island, false)));
},
};
Expand Down
16 changes: 15 additions & 1 deletion packages/astro/src/runtime/server/render/instruction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,32 @@ export type RenderHeadInstruction = {
type: 'head';
};

/**
* Render a renderer-specific hydration script before the first component of that
* framework
*/
export type RendererHydrationInstruction = {
type: 'renderer-hydration';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call it hydration-script? At the end, that's the actual meaning of this instruction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

rendererName: string;
render: () => string;
};

export type MaybeRenderHeadInstruction = {
type: 'maybe-head';
};

export type RenderInstruction =
| RenderDirectiveInstruction
| RenderHeadInstruction
| MaybeRenderHeadInstruction;
| MaybeRenderHeadInstruction
| RendererHydrationInstruction;

export function createRenderInstruction(
instruction: RenderDirectiveInstruction
): RenderDirectiveInstruction;
export function createRenderInstruction(
instruction: RendererHydrationInstruction
): RendererHydrationInstruction;
export function createRenderInstruction(instruction: RenderHeadInstruction): RenderHeadInstruction;
export function createRenderInstruction(
instruction: MaybeRenderHeadInstruction
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Based on reproduction from https://github.com/withastro/astro/issues/6912

import { For, Match, Switch } from 'solid-js';

export default function Counter(props) {
return (
<For each={[1, 2, 3, 4]}>
{(page) => {
return (
<Switch>
<Match when={page % 2 === 0}>
<button
onClick={() => {
console.log(page);
}}
>
even {page}
</button>
</Match>
<Match when={page % 2 === 1}>
<button
onClick={() => {
console.log(page);
}}
>
odd {page}
</button>
</Match>
</Switch>
);
}}
</For>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Based on reproduction from https://github.com/withastro/astro/issues/6912

import { lazy } from 'solid-js';

export const LazyCounter = lazy(() => import('./Counter'));
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { createResource, createSignal, createUniqueId, ErrorBoundary, Show } from 'solid-js';

// It may be good to try short and long sleep times.
// But short is faster for testing.
const SLEEP_MS = 10;

const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms));

export function AsyncComponent(props) {
const id = createUniqueId();

const [data] = createResource(async () => {
// console.log("Start rendering async component " + props.title);
await sleep(props.delay ?? SLEEP_MS);
// console.log("Finish rendering async component " + props.title);
return 'Async result for component id=' + id;
});

const [show, setShow] = createSignal(false);

return (
<div data-name="AsyncComponent" style={{ border: 'black solid 1px', padding: '4px' }}>
{'title=' + (props.title ?? '(none)') + ' '}
{'id=' + id + ' '}
<span>{data()}</span>{' '}
<button
type="button"
disabled={show()}
onClick={() => {
setShow(true);
}}
>
Show children
</button>
{/* NOTE: The props.children are intentionally hidden by default
to simulate a situation where hydration script might not
be injected in the right spot. */}
<Show when={show()}>{props.children ?? 'Empty'}</Show>
</div>
);
}

export function AsyncErrorComponent() {
const [data] = createResource(async () => {
await sleep(SLEEP_MS);
throw new Error('Async error thrown!');
});

return <div>{data()}</div>;
}

export function AsyncErrorInErrorBoundary() {
return (
<ErrorBoundary fallback={<div>Async error boundary fallback</div>}>
<AsyncErrorComponent />
</ErrorBoundary>
);
}

export function SyncErrorComponent() {
throw new Error('Sync error thrown!');
}

export function SyncErrorInErrorBoundary() {
return (
<ErrorBoundary fallback={<div>Sync error boundary fallback</div>}>
<SyncErrorComponent />
</ErrorBoundary>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
import { AsyncComponent } from './async-components.jsx';

await new Promise((resolve) => setTimeout(resolve, Astro.props.delay));
---

<AsyncComponent client:load />
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
import Defer from '../components/defer.astro';
---

<html>
<head><title>Solid</title></head>
<body>
<Defer delay={50} />
<Defer delay={10} />
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
import { AsyncComponent } from '../components/async-components.jsx';
---

<html>
<head><title>Nested Test</title></head>
<body>
<div>
<AsyncComponent client:load title="level-a">
<AsyncComponent client:load title="level-a-a" />
<AsyncComponent client:load title="level-a-b">
<AsyncComponent client:load title="level-a-b-a" />
</AsyncComponent>
<AsyncComponent client:load title="level-a-2" />
</AsyncComponent>
</div>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
import {
AsyncErrorInErrorBoundary,
SyncErrorInErrorBoundary,
} from '../components/async-components.jsx';
---

<html>
<head><title>Solid</title></head>
<body>
<div>
<!--
Error boundary in hydrating component may generate scripts script:
https://github.com/ryansolid/dom-expressions/blob/6746f048c4adf4d4797276f074dd2d487654796a/packages/dom-expressions/src/server.js#L24
So make sure that the hydration script is generated on this page.
-->
<AsyncErrorInErrorBoundary client:load />
<SyncErrorInErrorBoundary client:load />
</div>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
import { LazyCounter } from '../components/LazyCounter.jsx';
import { AsyncComponent } from '../components/async-components.jsx';
---

<html>
<head><title>Solid</title></head>
<body>
<div>
<!-- client:load should generate exactly one hydration script per page -->
<AsyncComponent client:load />
<AsyncComponent client:load />
<!-- Lazy copmonents should render consistently, even on first render. -->
<LazyCounter client:load />
</div>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
---
import {
AsyncErrorInErrorBoundary,
SyncErrorInErrorBoundary,
// AsyncErrorComponent,
// SyncErrorComponent,
} from '../components/async-components.jsx';
---

<html>
<head><title>Solid</title></head>
<body>
<div>
<!-- Async errors should be caught by ErrorBoundary -->
<AsyncErrorInErrorBoundary />
<!-- Sync errors should be caught by ErrorBoundary -->
<SyncErrorInErrorBoundary />

<!-- Error not wrapped in ErrorBoundary should bubble up to Astro renderToStaticMarkup() function. -->
<!-- <AsyncErrorComponent /> -->
<!-- <SyncErrorComponent /> -->
</div>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
import { AsyncComponent } from '../components/async-components.jsx';
import { LazyCounter } from '../components/LazyCounter.jsx';
---

<html>
<head><title>Solid</title></head>
<body>
<div>
<!-- Static component should not create any hydration scripts -->
<AsyncComponent />
<!-- Lazy copmonents should render consistently, even on first render. -->
<LazyCounter />
</div>
</body>
</html>
Loading
Loading