Skip to content

fix: head duplication when binding is present #9124

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

Merged
merged 5 commits into from
Aug 28, 2023
Merged

fix: head duplication when binding is present #9124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a23277
chore: render head output in playground
gtm-nayan Aug 20, 2023
a1e60a3
add tests
gtm-nayan Aug 20, 2023
cfdc519
fix head duplication with binding present
gtm-nayan Aug 20, 2023
f0e1f69
changeset
gtm-nayan Aug 20, 2023
adea603
Merge branch 'master' into fix-binding-head-duplicate
gtm-nayan Aug 20, 2023
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/stale-terms-sing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: head duplication when binding is present
4 changes: 4 additions & 0 deletions packages/svelte/src/compiler/compile/render_ssr/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,13 @@ export default function ssr(component, options) {
? b`
let $$settled;
let $$rendered;
let #previous_head = $$result.head;

do {
$$settled = true;
// $$result.head is mutated by the literal expression
// need to reset it if we're looping back to prevent duplication
$$result.head = #previous_head;
Copy link
Member

Choose a reason for hiding this comment

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

Still don't fully understand the fix - how is the binding mutating the head?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Binding doesn't mutate it, but the emitted code is like

`${($$result.head += 'head contents', '')} body contents`

so if the loop isn't settled in one go, the next iteration will add the head contents again and so on

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see - I guess it makes sense then to only use the first head. Can there ever be a situation where this would prevent other head elements from showing up? Like, if there's some component inside an if block that contains a head element, will that be part of the first loop already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fine, the child component inside the if will get reexecuted after resetting the head

Copy link
Member

Choose a reason for hiding this comment

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

So what constitutes to a rerun then? Only bindings? TBH I never looked into that part of the code base much. I just want to rule out that we don't lose head content that would otherwise be rightfully added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

svelte/packages/svelte/src/compiler/compile/render_ssr/index.js

Line 143 in adea603

const main = renderer.has_bindings

Looks like it's only bindings


${reactive_declarations}

Expand Down
3 changes: 3 additions & 0 deletions ...ages/svelte/test/server-side-rendering/samples/head-no-duplicates-with-binding/Foo.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<script>
export let bar = null
</script>
2 changes: 2 additions & 0 deletions ...te/test/server-side-rendering/samples/head-no-duplicates-with-binding/_expected-head.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<link rel="canonical" href="/test">
<meta name="description" content="test">
Empty file added .../svelte/test/server-side-rendering/samples/head-no-duplicates-with-binding/_expected.html
View file
Open in desktop
Empty file.
11 changes: 11 additions & 0 deletions ...ges/svelte/test/server-side-rendering/samples/head-no-duplicates-with-binding/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<script>
import Foo from './Foo.svelte';
let bar;
</script>

<svelte:head>
<link rel="canonical" href="/test" />
<meta name="description" content="test" />
</svelte:head>

<Foo bind:bar />