Fix island deduplication ignoring props. (#2825)

* Fix island deduplication ignoring props. Re-resolves an issue initially patched in #846 but seemingly lost in the 0.21.0 mega-merge (d84bfe7). This change makes the component render step account for all props, even if they don't affect the generated HTML, when deduplicating island mount. * Fix React component test using different rendered props to test deduplication. * fix: improve serialization support for non-JSON objects Co-authored-by: Nate Moore <nate@skypack.dev>
withastro · Mar 18, 2022 · 1cd7184 · 1cd7184
1 parent 0af017b
commit 1cd7184
Show file tree

Hide file tree

Showing 8 changed files with 41 additions and 15 deletions.
diff --git a/.changeset/silent-walls-tan.md b/.changeset/silent-walls-tan.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix island deduplication ignoring props.Re-resolves an issue initially patched in https://github.com/withastro/astro/pull/846 but seemingly lost in the 0.21.0 mega-merge (https://github.com/withastro/astro/commit/d84bfe719a546ad855640338d5ed49ad3aa4ccb4).This change makes the component render step account for all props, even if they don't affect the generated HTML, when deduplicating island mounts.
diff --git a/packages/astro/src/runtime/server/index.ts b/packages/astro/src/runtime/server/index.ts
@@ -3,7 +3,7 @@ import type { AstroGlobalPartial, SSRResult, SSRElement } from '../../@types/ast
 import type { AstroRequest } from '../../core/render/request';
 
 import shorthash from 'shorthash';
-import { extractDirectives, generateHydrateScript } from './hydration.js';
+import { extractDirectives, generateHydrateScript, serializeProps } from './hydration.js';
 import { serializeListValue } from './util.js';
 import { escapeHTML, HTMLString, markHTMLString } from './escape.js';
 
@@ -278,8 +278,8 @@ If you're still stuck, please open an issue on GitHub or join us at https://astr
 		return markHTMLString(html.replace(/\<\/?astro-fragment\>/g, ''));
 	}
 
-	// Include componentExport name and componentUrl in hash to dedupe identical islands
-	const astroId = shorthash.unique(`<!--${metadata.componentExport!.value}:${metadata.componentUrl}-->\n${html}`);
+	// Include componentExport name, componentUrl, and props in hash to dedupe identical islands
+	const astroId = shorthash.unique(`<!--${metadata.componentExport!.value}:${metadata.componentUrl}-->\n${html}\n${serializeProps(props)}`);
 
 	// Rather than appending this inline in the page, puts this into the `result.scripts` set that will be appended to the head.
 	// INVESTIGATE: This will likely be a problem in streaming because the `<head>` will be gone at this point.

diff --git a/packages/astro/test/fixtures/react-component/src/components/Hello.jsx b/packages/astro/test/fixtures/react-component/src/components/Hello.jsx
@@ -1,5 +1,5 @@
 import React from 'react';
 
-export default function ({ name }) {
-  return <h2 id="react-h2">Hello {name}!</h2>;
+export default function ({ name, unused }) {
+  return <h2 id={`react-${name}`}>Hello {name}!</h2>;
 }
diff --git a/packages/astro/test/fixtures/react-component/src/pages/index.astro b/packages/astro/test/fixtures/react-component/src/pages/index.astro
@@ -17,7 +17,12 @@ const someProps = {
     <!-- Head Stuff -->
   </head>
   <body>
-    <Hello name="world" />
+    <Hello name="static" />
+    <Hello name="load" client:load />
+    <!-- Test island deduplication, i.e. same UID as the component above. -->
+    <Hello name="load" client:load />
+    <!-- Test island deduplication account for non-render affecting props. -->
+    <Hello name="load" unused="" client:load />
     <Later name="baby" />
     <ArrowFunction />
     <PropsSpread {...someProps}/>

diff --git a/packages/astro/test/fixtures/vue-component/src/components/Counter.vue b/packages/astro/test/fixtures/vue-component/src/components/Counter.vue
@@ -20,12 +20,17 @@ export default {
     start: {
       type: String,
       required: true
+    },
+    stepSize: {
+      type: String,
+      default: "1"
     }
   },
   setup(props) {
     const count = ref(parseInt(props.start))
-    const add = () => (count.value = count.value + 1);
-    const subtract = () => (count.value = count.value - 1);
+    const stepSize = ref(parseInt(props.stepSize))
+    const add = () => (count.value = count.value + stepSize.value);
+    const subtract = () => (count.value = count.value - stepSize.value);
     return {
       count,
       add,

diff --git a/packages/astro/test/fixtures/vue-component/src/pages/index.astro b/packages/astro/test/fixtures/vue-component/src/pages/index.astro
@@ -20,6 +20,10 @@ import Counter from '../components/Counter.vue'
     <main>
       <Counter start="0">SSR Rendered, No Client</Counter>
       <Counter start="1" client:load>SSR Rendered, client:load</Counter>
+      <!-- Test island deduplication, i.e. same UID as the component above. -->
+      <Counter start="1" client:load>SSR Rendered, client:load</Counter>
+      <!-- Test island deduplication account for non-render affecting props. -->
+      <Counter start="1" step-size="2" client:load>SSR Rendered, client:load</Counter>
       <Counter start="10" client:idle>SSR Rendered, client:idle</Counter>
       <!-- Test that two client:visibles have unique uids -->
       <Counter start="100" client:visible>SSR Rendered, client:visible</Counter>

diff --git a/packages/astro/test/react-component.test.js b/packages/astro/test/react-component.test.js
@@ -25,10 +25,10 @@ describe('React Components', () => {
 			const $ = cheerio.load(html);
 
 			// test 1: basic component renders
-			expect($('#react-h2').text()).to.equal('Hello world!');
+			expect($('#react-static').text()).to.equal('Hello static!');
 
 			// test 2: no reactroot
-			expect($('#react-h2').attr('data-reactroot')).to.equal(undefined);
+			expect($('#react-static').attr('data-reactroot')).to.equal(undefined);
 
 			// test 3: Can use function components
 			expect($('#arrow-fn-component')).to.have.lengthOf(1);
@@ -44,6 +44,13 @@ describe('React Components', () => {
 
 			// test 7: Can use Pure components
 			expect($('#pure')).to.have.lengthOf(1);
+
+			// test 8: Check number of islands
+			expect($('astro-root[uid]')).to.have.lengthOf(5);
+
+			// test 9: Check island deduplication
+			const uniqueRootUIDs = new Set($('astro-root').map((i, el) => $(el).attr('uid')));
+			expect(uniqueRootUIDs.size).to.equal(4);
 		});
 
 		it('Can load Vue', async () => {

diff --git a/packages/astro/test/vue-component.test.js b/packages/astro/test/vue-component.test.js
@@ -29,17 +29,17 @@ describe('Vue component', () => {
 				.map((el) => $(el).text());
 
 			// test 1: renders all components correctly
-			expect(allPreValues).to.deep.equal(['0', '1', '10', '100', '1000']);
+			expect(allPreValues).to.deep.equal(['0', '1', '1', '1', '10', '100', '1000']);
 
 			// test 2: renders 3 <astro-root>s
-			expect($('astro-root')).to.have.lengthOf(4);
+			expect($('astro-root')).to.have.lengthOf(6);
 
 			// test 3: all <astro-root>s have uid attributes
-			expect($('astro-root[uid]')).to.have.lengthOf(4);
+			expect($('astro-root[uid]')).to.have.lengthOf(6);
 
-			// test 5: all <astro-root>s have unique uid attributes
+			// test 5: components with identical render output and props have been deduplicated
 			const uniqueRootUIDs = $('astro-root').map((i, el) => $(el).attr('uid'));
-			expect(new Set(uniqueRootUIDs).size).to.equal(4);
+			expect(new Set(uniqueRootUIDs).size).to.equal(5);
 		});
 	});