diff options
| author |  Hlynur Sveinbjornsson <hlynur@reincubate.com>
| 2022-03-18 09:00:14 -0400 |
|---|---|---|
| committer |  GitHub <noreply@github.com>
| 2022-03-18 09:00:14 -0400 |
| commit | 1cd7184ca6fa6e60a69918e461f42c055e8a795e (patch) | |
| tree | fca4f104430184b13ae5e823220f9671a477e416 | |
| parent | 0af017b9fc8abc8aa101ada7e28e9e738d166807 (diff) | |
| download | astro-1cd7184ca6fa6e60a69918e461f42c055e8a795e.tar.gz astro-1cd7184ca6fa6e60a69918e461f42c055e8a795e.tar.zst astro-1cd7184ca6fa6e60a69918e461f42c055e8a795e.zip | |
Fix island deduplication ignoring props. (#2825)
* 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 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>
8 files changed, 41 insertions, 15 deletions
diff --git a/.changeset/silent-walls-tan.md b/.changeset/silent-walls-tan.md new file mode 100644 index 000000000..1eb3a97a4 --- /dev/null +++ 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 index 7d7088ad8..da510f2fb 100644 --- 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 index 4b6c416a9..4c241162d 100644 --- 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 index 597a4a546..049b5fb6a 100644 --- 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 index 49d2936ed..8cbb0f550 100644 --- 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 index eed6624cc..7cae724dc 100644 --- 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 index a05f76cf9..8e90f88ae 100644 --- 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 index 46edcfbac..7b226b911 100644 --- 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); }); }); |