fix : reactivity bug with single script in svelte:head

[adiguba]

template_with_script() is buggy when it contains only one <script> element, because the run_scripts() will clone and replace the scripts.

There no problem when using a DocumentFragment because they are replaced immediately inside the fragment.
But with a single node the element cannot be replaced, and it's done later with queue_micro_task().

=> So the cloned script is put on the DOM, but template_with_script() will return the previous script element (so reactivity is broken).

See #13378

Should I make a test for that ?

Svelte 5 rewrite

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

⚠️ No Changeset found

Latest commit: 2283b0c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[adiguba]

For the fix, I force the template_with_script() to always use a fragment, so run_scripts() can replace the elements.

[paoloricciuti]

Should I make a test for that ?

I think we should add a test for this but i'm not sure JSDom will behave correctly but is worth trying.

However i'm thinking...could a better and simpler solution be returning the cloned script from run_scripts and reassigning it in template_with_script?

[adiguba]

However i'm thinking...could a better and simpler solution be returning the cloned script from run_scripts and reassigning it in template_with_script?

I tried that on the previous PR that I closed, and I think it's more verbose : https://github.com/sveltejs/svelte/pull/13385/files

[paoloricciuti]

I tried that on the previous PR that I closed, and I think it's more verbose : https://github.com/sveltejs/svelte/pull/13385/files

But if you are returning the right node why are you still heavily changing the logic inside run_scripts? The moment you return the right element you can just keep the logic the same there

[adiguba]

You are right, I redid the code and it is much simpler.

But in the main time I think I found another bug : I don't understand the if (first)trick in the original code :

export function template_with_script(content, flags) {
	var first = true;
	var fn = template(content, flags);

	return () => {
		if (hydrating) return fn();

		var node = /** @type {Element | DocumentFragment} */ (fn());

		if (first) {
			first = false;
			run_scripts(node);
		}

		return node;
	};
}

Why run_scripts() is only executed on the first template ?
It won't work when executed on a {#each}...

Exemple : https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACnVRTW-DMAz9K1Y2ibZDRW2nHSgwbdIO0w47dLdSqSExLRUElJhqCPHfl9BJaw9LlA_72X7PSc_yokTDwm3PFK-QheylaZjPqGucYc5YElrb1K0WzhMZoYuGklSllLdKUFErOBmjxcSuKfQOSEkjtVqBJznxkPCbghM_80vuOuMGnx598OABMqr5mLl2iYPd7CFqZQgu0RstDMSwhYUPSx9WsJtXvJkoiJNf3r2sRVuhovkB6a1Ed33t3uXE0_e9GrzpvFAK9ZdVYQt5m7EsOAg-P7z1fuq4o-CvMRVd-g6PyOXYaX-HXByvFXEDltvqBTtS-n0W54t7B9iATgmQmKNOrouTm5GhrsQEghkYavMcqIbWIOSaH5x8mAVgk8agkT9w_MMo81aaijIoZJwyvUhZ8hwFWRJlOkiugOV_wOoGsL9c1bLIC5QsJN3isBt-AG3a5jYgAgAA

Finally I tried to do a test, without success.
How to force the test to wait for the <script> to load?

[paoloricciuti]

Why run_scripts() is only executed on the first template ?

It's not on the first template, it's the first time the template is used. I think I saw some comments somewhere about why we don't want to re-run the scripts after the first time. I'm not at the desk now but you can try searching some PR to see if there was a discussion.

Finally I tried to do a test, without success.
How to force the test to wait for the <script> to load?

Yeah that's why I said JSdom could be tricky to work with in this case...if you search the codebase for JSDom there should be some comment but I can't remember if it was related to scripts

[adiguba]

I found the PR where the if (first) was added : #10954
Change : 326e2b4#diff-63b0f1ec3f3906550c70ebfc2825fddcdd0dd6bfb5d6183eef4b9b489cd13d0a

But I don't see any discussion about that...

[paoloricciuti]

Tbf i think this was probably a bug...i think it all stemmed from the fact that initially the fragment was cached so script only ran once but i think we should probably remove it. But I would love for @dummdidumm or @Rich-Harris to confirm this.

[dummdidumm]

This will undo some of the other fixes (which sadly can't be really tested). I'm a bit surprised this issue isn't handled by the existing logic of waiting one micro task

[paoloricciuti]

This will undo some of the other fixes (which sadly can't be really tested). I'm a bit surprised this issue isn't handled by the existing logic of waiting one micro task

That's exactly the bug: we are using set_attribute but set_attribute run before the microtask so when the script is cloned it doesn't have the attribute yet, then the attribute is set and then the script with the attribute is replaced with the one without the attribute.

I guess the fix is to move the cloning logic into replace so that we clone it immediately before replacing it without loosing any possible modification we made to it

[dummdidumm]

There's other problems with the replacement logic after a tick I just realized: Svelte's walking mechanism will be finished by this time and so it will have a reference to an outdated script node. If you ever change a value of that script it won't work.
I think we need to add a comment to sole script tags to ensure we have a parent to replace within synchronously.

[paoloricciuti]

There's other problems with the replacement logic after a tick I just realized: Svelte's walking mechanism will be finished by this time and so it will have a reference to an outdated script node. If you ever change a value of that script it won't work. I think we need to add a comment to sole script tags to ensure we have a parent to replace within synchronously.

Yeah exactly the bug.