-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Fix island deduplication ignoring props. #2825
Conversation
Re-resolves an issue initially patched in withastro#846 but seemingly lost in the 0.21.0 mega-merge (withastro@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.
🦋 Changeset detectedLatest commit: 63a1939 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 🎉 This is a huge help.
Straight-forward fix, extensive test coverage. I love to see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Called out an edge case this PR doesn't cover. Still, it should solve a majority of issues!
// 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
* Fix island deduplication ignoring props. Re-resolves an issue initially patched in withastro#846 but seemingly lost in the 0.21.0 mega-merge (withastro@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>
Re-resolves an issue initially patched in #846 but seemingly lost in the 0.21.0 mega-merge (d84bfe7) or somewhere along the way there.
This change makes the component render step account for all props, even if they don't affect the generated HTML, when deduplicating island mounts.
This resolves the following issues:
client:only
is broken when component used more than once #2725Changes
astroId
hash generation during component rendering.Testing
Added tests for island mount deduplication to Vue and React component tests, both to confirm deduplication works as expected for identical component usage and that it distinguishes between component with different props but otherwise identical generated markup.
Docs
There are no changes to make to user-facing documentation.