From f5fcaa47670e1863372befb1db834e1b37d98111 Mon Sep 17 00:00:00 2001 From: Nate Moore Date: Tue, 16 Aug 2022 13:32:06 -0400 Subject: [PATCH 1/6] fix(#4332): add helpful error on cyclic references --- .../astro/src/runtime/server/hydration.ts | 2 +- .../src/runtime/server/render/component.ts | 4 +-- .../astro/src/runtime/server/serialize.ts | 35 +++++++++++++------ 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/packages/astro/src/runtime/server/hydration.ts b/packages/astro/src/runtime/server/hydration.ts index fea251b79272..30688237acb6 100644 --- a/packages/astro/src/runtime/server/hydration.ts +++ b/packages/astro/src/runtime/server/hydration.ts @@ -146,7 +146,7 @@ export async function generateHydrateScript( if (renderer.clientEntrypoint) { island.props['component-export'] = componentExport.value; island.props['renderer-url'] = await result.resolve(decodeURI(renderer.clientEntrypoint)); - island.props['props'] = escapeHTML(serializeProps(props)); + island.props['props'] = escapeHTML(serializeProps(props, metadata)); } island.props['ssr'] = ''; diff --git a/packages/astro/src/runtime/server/render/component.ts b/packages/astro/src/runtime/server/render/component.ts index 066a863539ce..389fdecf4fac 100644 --- a/packages/astro/src/runtime/server/render/component.ts +++ b/packages/astro/src/runtime/server/render/component.ts @@ -302,9 +302,7 @@ If you're still stuck, please open an issue on GitHub or join us at https://astr // Include componentExport name, componentUrl, and props in hash to dedupe identical islands const astroId = shorthash( - `\n${html}\n${serializeProps( - props - )}` + `\n${html}\n${serializeProps(props, metadata)}` ); const island = await generateHydrateScript( diff --git a/packages/astro/src/runtime/server/serialize.ts b/packages/astro/src/runtime/server/serialize.ts index 21996a9320d1..4de21c08daaf 100644 --- a/packages/astro/src/runtime/server/serialize.ts +++ b/packages/astro/src/runtime/server/serialize.ts @@ -1,3 +1,7 @@ +import type { + AstroComponentMetadata, +} from '../../@types/astro'; + type ValueOf = T[keyof T]; const PROP_TYPE = { @@ -11,19 +15,25 @@ const PROP_TYPE = { URL: 7, }; -function serializeArray(value: any[]): any[] { - return value.map((v) => convertToSerializedForm(v)); +function serializeArray(value: any[], metadata: AstroComponentMetadata): any[] { + return value.map((v) => convertToSerializedForm(v, metadata)); } -function serializeObject(value: Record): Record { +function serializeObject(value: Record, metadata: AstroComponentMetadata): Record { + if (cyclicRefs.has(value)) { + throw new Error(`Cyclic reference detected while serializing props for <${metadata.displayName} client:${metadata.hydrate}>! + +Cyclic references cannot be safely serialized for client-side usage. Please remove the cyclic reference.`) + } + cyclicRefs.add(value); return Object.fromEntries( Object.entries(value).map(([k, v]) => { - return [k, convertToSerializedForm(v)]; + return [k, convertToSerializedForm(v, metadata)]; }) ); } -function convertToSerializedForm(value: any): [ValueOf, any] { +function convertToSerializedForm(value: any, metadata: AstroComponentMetadata): [ValueOf, any] { const tag = Object.prototype.toString.call(value); switch (tag) { case '[object Date]': { @@ -33,10 +43,10 @@ function convertToSerializedForm(value: any): [ValueOf, any] { return [PROP_TYPE.RegExp, (value as RegExp).source]; } case '[object Map]': { - return [PROP_TYPE.Map, JSON.stringify(serializeArray(Array.from(value as Map)))]; + return [PROP_TYPE.Map, JSON.stringify(serializeArray(Array.from(value as Map), metadata))]; } case '[object Set]': { - return [PROP_TYPE.Set, JSON.stringify(serializeArray(Array.from(value as Set)))]; + return [PROP_TYPE.Set, JSON.stringify(serializeArray(Array.from(value as Set), metadata))]; } case '[object BigInt]': { return [PROP_TYPE.BigInt, (value as bigint).toString()]; @@ -45,11 +55,11 @@ function convertToSerializedForm(value: any): [ValueOf, any] { return [PROP_TYPE.URL, (value as URL).toString()]; } case '[object Array]': { - return [PROP_TYPE.JSON, JSON.stringify(serializeArray(value))]; + return [PROP_TYPE.JSON, JSON.stringify(serializeArray(value, metadata))]; } default: { if (value !== null && typeof value === 'object') { - return [PROP_TYPE.Value, serializeObject(value)]; + return [PROP_TYPE.Value, serializeObject(value, metadata)]; } else { return [PROP_TYPE.Value, value]; } @@ -57,6 +67,9 @@ function convertToSerializedForm(value: any): [ValueOf, any] { } } -export function serializeProps(props: any) { - return JSON.stringify(serializeObject(props)); +let cyclicRefs = new WeakSet(); +export function serializeProps(props: any, metadata: AstroComponentMetadata) { + const serialized = JSON.stringify(serializeObject(props, metadata)); + cyclicRefs = new WeakSet(); + return serialized; } From ece38a887bb42955506c8220ae1728ee87004dc4 Mon Sep 17 00:00:00 2001 From: Nate Moore Date: Tue, 16 Aug 2022 13:34:15 -0400 Subject: [PATCH 2/6] chore: add changeset --- .changeset/giant-beds-beam.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/giant-beds-beam.md diff --git a/.changeset/giant-beds-beam.md b/.changeset/giant-beds-beam.md new file mode 100644 index 000000000000..0aaae5db6f12 --- /dev/null +++ b/.changeset/giant-beds-beam.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Add cyclic ref detection when serializing props From cbf2dc6dd9c9ae7cf8a1fe0aa0191d34dcd4df95 Mon Sep 17 00:00:00 2001 From: Nate Moore Date: Tue, 16 Aug 2022 13:49:08 -0400 Subject: [PATCH 3/6] test(e2e): add cyclic reference test --- packages/astro/e2e/error-cyclic.test.js | 24 +++++++++++++++++++ .../e2e/fixtures/error-cyclic/package.json | 9 +++++++ .../src/components/PreactCounter.tsx | 17 +++++++++++++ .../error-cyclic/src/pages/index.astro | 8 +++++++ 4 files changed, 58 insertions(+) create mode 100644 packages/astro/e2e/error-cyclic.test.js create mode 100644 packages/astro/e2e/fixtures/error-cyclic/package.json create mode 100644 packages/astro/e2e/fixtures/error-cyclic/src/components/PreactCounter.tsx create mode 100644 packages/astro/e2e/fixtures/error-cyclic/src/pages/index.astro diff --git a/packages/astro/e2e/error-cyclic.test.js b/packages/astro/e2e/error-cyclic.test.js new file mode 100644 index 000000000000..78c3bd1ead12 --- /dev/null +++ b/packages/astro/e2e/error-cyclic.test.js @@ -0,0 +1,24 @@ +import { expect } from '@playwright/test'; +import { testFactory, getErrorOverlayMessage } from './test-utils.js'; + +const test = testFactory({ root: './fixtures/error-cyclic/' }); + +let devServer; + +test.beforeEach(async ({ astro }) => { + devServer = await astro.startDevServer(); +}); + +test.afterEach(async ({ astro }) => { + await devServer.stop(); + astro.resetAllFiles(); +}); + +test.describe('Error: Cyclic Reference', () => { + test('overlay', async ({ page, astro }) => { + await page.goto(astro.resolveUrl('/')); + + const message = await getErrorOverlayMessage(page); + expect(message).toMatch('Cyclic reference'); + }); +}); diff --git a/packages/astro/e2e/fixtures/error-cyclic/package.json b/packages/astro/e2e/fixtures/error-cyclic/package.json new file mode 100644 index 000000000000..46cce7a02f66 --- /dev/null +++ b/packages/astro/e2e/fixtures/error-cyclic/package.json @@ -0,0 +1,9 @@ +{ + "name": "@e2e/error-cyclic", + "version": "0.0.0", + "private": true, + "dependencies": { + "astro": "workspace:*", + "@astrojs/preact": "workspace:*" + } +} diff --git a/packages/astro/e2e/fixtures/error-cyclic/src/components/PreactCounter.tsx b/packages/astro/e2e/fixtures/error-cyclic/src/components/PreactCounter.tsx new file mode 100644 index 000000000000..b0570046c4f8 --- /dev/null +++ b/packages/astro/e2e/fixtures/error-cyclic/src/components/PreactCounter.tsx @@ -0,0 +1,17 @@ +import { useState } from 'preact/hooks'; + +/** a counter written in Preact */ +export function PreactCounter({ children, id }) { + const [count, setCount] = useState(0); + const add = () => setCount((i) => i + 1); + const subtract = () => setCount((i) => i - 1); + + return ( +
+ + 
{count}
+ +
{children}
+
+ ); +} diff --git a/packages/astro/e2e/fixtures/error-cyclic/src/pages/index.astro b/packages/astro/e2e/fixtures/error-cyclic/src/pages/index.astro new file mode 100644 index 000000000000..32d68994bf6a --- /dev/null +++ b/packages/astro/e2e/fixtures/error-cyclic/src/pages/index.astro @@ -0,0 +1,8 @@ +--- +import { PreactCounter } from '../components/PreactCounter' + +const cycle: any = { foo: ['bar'] } +cycle.foo.push(cycle) +--- + + From a3803caceb752a5a879aa7e4c1b0e7861a884d14 Mon Sep 17 00:00:00 2001 From: Nate Moore Date: Tue, 16 Aug 2022 15:21:49 -0400 Subject: [PATCH 4/6] test(e2e): add preact integration --- packages/astro/e2e/fixtures/error-cyclic/astro.config.mjs | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 packages/astro/e2e/fixtures/error-cyclic/astro.config.mjs diff --git a/packages/astro/e2e/fixtures/error-cyclic/astro.config.mjs b/packages/astro/e2e/fixtures/error-cyclic/astro.config.mjs new file mode 100644 index 000000000000..08916b1fea78 --- /dev/null +++ b/packages/astro/e2e/fixtures/error-cyclic/astro.config.mjs @@ -0,0 +1,7 @@ +import { defineConfig } from 'astro/config'; +import preact from '@astrojs/preact'; + +// https://astro.build/config +export default defineConfig({ + integrations: [preact()], +}); From bf317fde1ff20506f76339beeafba4caf98868e4 Mon Sep 17 00:00:00 2001 From: Nate Moore Date: Tue, 6 Sep 2022 17:11:51 -0500 Subject: [PATCH 5/6] chore: update lockfile --- pnpm-lock.yaml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a48d94cd43ab..948eae742881 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -585,6 +585,14 @@ importers: '@astrojs/vue': link:../../../../integrations/vue astro: link:../../.. + packages/astro/e2e/fixtures/error-cyclic: + specifiers: + '@astrojs/preact': workspace:* + astro: workspace:* + dependencies: + '@astrojs/preact': link:../../../../integrations/preact + astro: link:../../.. + packages/astro/e2e/fixtures/error-react-spectrum: specifiers: '@adobe/react-spectrum': ^3.18.0 @@ -972,14 +980,6 @@ importers: '@astrojs/tailwind': link:../../../../integrations/tailwind astro: link:../../.. - packages/astro/e2e/fixtures/third-party-astro: - specifiers: - astro: workspace:* - astro-embed: ^0.1.1 - dependencies: - astro: link:../../.. - astro-embed: 0.1.1_astro@packages+astro - packages/astro/e2e/fixtures/ts-resolution: specifiers: '@astrojs/react': workspace:* From ff0b17e78f9d67608f9b03392878fda471db3463 Mon Sep 17 00:00:00 2001 From: Nate Moore Date: Tue, 6 Sep 2022 17:24:56 -0500 Subject: [PATCH 6/6] fix: ensure vite client is loaded for 500 responses --- packages/astro/src/vite-plugin-astro-server/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/astro/src/vite-plugin-astro-server/index.ts b/packages/astro/src/vite-plugin-astro-server/index.ts index 525ec0a86b0b..883e3c91d14d 100644 --- a/packages/astro/src/vite-plugin-astro-server/index.ts +++ b/packages/astro/src/vite-plugin-astro-server/index.ts @@ -120,6 +120,7 @@ async function handle500Response( ) { res.on('close', () => setTimeout(() => viteServer.ws.send(getViteErrorPayload(err)), 200)); if (res.headersSent) { + res.write(``) res.end(); } else { writeHtmlResponse(