Improve cyclic reference detection, now ignores non-cyclic shared references (#4684)

* fix: improve cyclic reference detection, now ignores references that are not parent/child

* fix: only track cyclic parents

Co-authored-by: Nate Moore <nate@astro.build>

.changeset/lovely-clocks-tease.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes regression introduced in [#4646](https://github.com/withastro/astro/pull/4646) with better cyclic reference detection

packages/astro/src/runtime/server/serialize.ts

@@ -13,30 +13,44 @@ const PROP_TYPE = {
 	URL: 7,
 };
 
-function serializeArray(value: any[], metadata: AstroComponentMetadata): any[] {
+function serializeArray(value: any[], metadata: AstroComponentMetadata | Record<string, any> = {}, parents = new WeakSet<any>()): any[] {
-	return value.map((v) => convertToSerializedForm(v, metadata));
+	if (parents.has(value)) {
-}
-
-function serializeObject(
-	value: Record<any, any>,
-	metadata: AstroComponentMetadata
-): Record<any, any> {
-	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);
+	parents.add(value);
-	return Object.fromEntries(
+	const serialized = value.map((v) => {
+		return convertToSerializedForm(v, metadata, parents)
+	});
+	parents.delete(value);
+	return serialized;
+}
+
+function serializeObject(
+	value: Record<any, any>,
+	metadata: AstroComponentMetadata | Record<string, any> = {},
+	parents = new WeakSet<any>()
+): Record<any, any> {
+	if (parents.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.`);
+	}
+	parents.add(value);
+	const serialized = Object.fromEntries(
 		Object.entries(value).map(([k, v]) => {
-			return [k, convertToSerializedForm(v, metadata)];
+			return [k, convertToSerializedForm(v, metadata, parents)];
 		})
 	);
+	parents.delete(value);
+	return serialized;
 }
 
 function convertToSerializedForm(
 	value: any,
-	metadata: AstroComponentMetadata
+	metadata: AstroComponentMetadata | Record<string, any> = {},
+	parents = new WeakSet<any>()
 ): [ValueOf<typeof PROP_TYPE>, any] {
 	const tag = Object.prototype.toString.call(value);
 	switch (tag) {
@@ -49,13 +63,13 @@ function convertToSerializedForm(
 		case '[object Map]': {
 			return [
 				PROP_TYPE.Map,
-				JSON.stringify(serializeArray(Array.from(value as Map<any, any>), metadata)),
+				JSON.stringify(serializeArray(Array.from(value as Map<any, any>), metadata, parents)),
 			];
 		}
 		case '[object Set]': {
 			return [
 				PROP_TYPE.Set,
-				JSON.stringify(serializeArray(Array.from(value as Set<any>), metadata)),
+				JSON.stringify(serializeArray(Array.from(value as Set<any>), metadata, parents)),
 			];
 		}
 		case '[object BigInt]': {
@@ -65,11 +79,11 @@ function convertToSerializedForm(
 			return [PROP_TYPE.URL, (value as URL).toString()];
 		}
 		case '[object Array]': {
-			return [PROP_TYPE.JSON, JSON.stringify(serializeArray(value, metadata))];
+			return [PROP_TYPE.JSON, JSON.stringify(serializeArray(value, metadata, parents))];
 		}
 		default: {
 			if (value !== null && typeof value === 'object') {
-				return [PROP_TYPE.Value, serializeObject(value, metadata)];
+				return [PROP_TYPE.Value, serializeObject(value, metadata, parents)];
 			} else {
 				return [PROP_TYPE.Value, value];
 			}
@@ -77,9 +91,7 @@ function convertToSerializedForm(
 	}
 }
 
-let cyclicRefs = new WeakSet<any>();
 export function serializeProps(props: any, metadata: AstroComponentMetadata) {
 	const serialized = JSON.stringify(serializeObject(props, metadata));
-	cyclicRefs = new WeakSet<any>();
 	return serialized;
 }

packages/astro/test/serialize.test.js

@@ -0,0 +1,67 @@
+import { expect } from 'chai';
+import { serializeProps } from '../dist/runtime/server/serialize.js';
+
+describe('serialize', () => {
+	it('serializes a plain value', () => {
+		const input = { a: 1 };
+		const output = `{"a":[0,1]}`;
+		expect(serializeProps(input)).to.equal(output);
+	})
+	it('serializes an array', () => {
+		const input = { a: [0] };
+		const output = `{"a":[1,"[[0,0]]"]}`;
+		expect(serializeProps(input)).to.equal(output);
+	})
+	it('serializes a regular expression', () => {
+		const input = { a: /b/ };
+		const output = `{"a":[2,"b"]}`;
+		expect(serializeProps(input)).to.equal(output);
+	})
+	it('serializes a Date', () => {
+		const input = { a: new Date(0) };
+		const output = `{"a":[3,"1970-01-01T00:00:00.000Z"]}`;
+		expect(serializeProps(input)).to.equal(output);
+	})
+	it('serializes a Map', () => {
+		const input = { a: new Map([[0, 1]]) };
+		const output = `{"a":[4,"[[1,\\"[[0,0],[0,1]]\\"]]"]}`;
+		expect(serializeProps(input)).to.equal(output);
+	})
+	it('serializes a Set', () => {
+		const input = { a: new Set([0, 1, 2, 3]) };
+		const output = `{"a":[5,"[[0,0],[0,1],[0,2],[0,3]]"]}`;
+		expect(serializeProps(input)).to.equal(output);
+	})
+	it('serializes a BigInt', () => {
+		const input = { a: BigInt('1') };
+		const output = `{"a":[6,"1"]}`;
+		expect(serializeProps(input)).to.equal(output);
+	})
+	it('serializes a URL', () => {
+		const input = { a: new URL('https://example.com/') };
+		const output = `{"a":[7,"https://example.com/"]}`;
+		expect(serializeProps(input)).to.equal(output);
+	})
+	it('cannot serialize a cyclic reference', () => {
+		const a = {}
+		a.b = a;
+		const input = { a };
+		expect(() => serializeProps(input)).to.throw(/cyclic/);
+	})
+	it('cannot serialize a cyclic array', () => {
+		const input = { foo: ['bar'] }
+		input.foo.push(input)
+		expect(() => serializeProps(input)).to.throw(/cyclic/);
+	})
+	it('cannot serialize a deep cyclic reference', () => {
+		const a = { b: {}};
+		a.b.c = a;
+		const input = { a };
+		expect(() => serializeProps(input)).to.throw(/cyclic/);
+	})
+	it('can serialize shared references that are not cyclic', () => {
+		const b = {}
+		const input = { a: { b, b }, b };
+		expect(() => serializeProps(input)).not.to.throw();
+	})
+})