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.

Sign up for GitHub

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

Fix island deduplication ignoring props. #2825

Merged
merged 3 commits into from
Mar 18, 2022
Merged

Fix island deduplication ignoring props. #2825

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a45a3f1
Fix island deduplication ignoring props.
hlynursmari1 Mar 17, 2022
99b046f
Fix React component test using different rendered props to test dedup…
hlynursmari1 Mar 17, 2022
63a1939
fix: improve serialization support for non-JSON objects
Mar 17, 2022
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
5 changes: 5 additions & 0 deletions .changeset/silent-walls-tan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 3 additions & 2 deletions packages/astro/src/runtime/server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,9 @@ 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 stringifiedProps = JSON.stringify(props);
Copy link
Contributor

@bholmesdev bholmesdev Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Note: this will not solve the issue for non-JSON props! When passing a function as a prop for instance:

---
import Counter from '../components/Counter.svelte';

function test() {
	return 'nice!'
}
---
<Counter client:visible nice="cool" testFn={test}>

...stringifiedProps will have this value:

'{"nice":"cool","class":"astro-3GOAJIZA"}'
// note testFn is missing

Doesn't look like JSON.stringify will throw on unrecognized keys, so we should be safe there. Just want to call out this doesn't fix 100% of use cases

Copy link
Member

@natemoo-re natemoo-re Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@bholmesdev Fixed in 63a1939. You're right that JSON.stringify didn't support everything that could be a prop. I switched to the serializeProps function for better parity.

We don't really support passing functions to components since they might reference something in a parent scope or have other dependencies...

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, never knew about serializeProps. This is great!

const astroId = shorthash.unique(`<!--${metadata.componentExport!.value}:${metadata.componentUrl}-->\n${html}\n${stringifiedProps}`);

// 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.
Expand Down
4 changes: 2 additions & 2 deletions packages/astro/test/fixtures/react-component/src/components/Hello.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>;
}
7 changes: 6 additions & 1 deletion packages/astro/test/fixtures/react-component/src/pages/index.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}/>
Expand Down
9 changes: 7 additions & 2 deletions packages/astro/test/fixtures/vue-component/src/components/Counter.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions packages/astro/test/fixtures/vue-component/src/pages/index.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
11 changes: 9 additions & 2 deletions packages/astro/test/react-component.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 () => {
Expand Down
10 changes: 5 additions & 5 deletions packages/astro/test/vue-component.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

Expand Down