fix: avoid warning for non reactive property in generated root

[paoloricciuti]

Closes sveltejs/svelte#12514

Since the new version of svelte 5 props are not reactive by default and only if wrapped in $bindable() so the generated root.svelte file was throwing this warning (and now that i think of it it might also cause some bug when the components actually changes...even tho looking at where they are used is always in an $effect with constructors so in theory whenever components changes constructors should also change)

I've added a test for this but i don't know if it's actually needed and also currently is passing even before the change because the tests seems to run with svelte 4 (and i don't see a way to run them with svelte 5). I can remove it if needed.

Also also if components really don't need to be reactive should we just silence the warning instead (and remove them from the effect)?

---

Please don't delete this checklist! 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
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

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

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

• Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

[changeset-bot]

🦋 Changeset detected

Latest commit: eaf1730

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

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

[paoloricciuti]

Mmm there are a bunch of unrelated tests failing ☹️

[benmccann]

Mmm there are a bunch of unrelated tests failing ☹️

Yeah, unfortunately the tests have been flaky for a long time. However, I think we just had a breakthrough on that front. I just sent #12487 to fix one of the main ones that's been failing. I wouldn't worry about the failing tests all that much, but you can rebase against main once that PR is merged

[paoloricciuti]

Mmm there are a bunch of unrelated tests failing ☹️

Yeah, unfortunately the tests have been flaky for a long time. However, I think we just had a breakthrough on that front. I just sent #12487 to fix one of the main ones that's been failing. I wouldn't worry about the failing tests all that much, but you can rebase against main once that PR is merged

Just merged back main (p.s. if you prefer rebasing i can undo the the merge and proceed with a rebase)

[paoloricciuti]

Also @benmccann does my test make sense or should i remove it if we don't run tests in svelte 5 yet?

[benmccann]

I use the term rebase out of habit, but merging main as you did is probably better. We just merged a couple more flaky test fixes, so you might want to do it once more

I'll let Rich and Simon do the actual review on this PR since they're the Svelte 5 experts

[paoloricciuti]

I use the term rebase out of habit, but merging main as you did is probably better. We just merged a couple more flaky test fixes, so you might want to do it once more

I'll let Rich and Simon do the actual review on this PR since they're the Svelte 5 experts

Thanks...merged main back again.

[dummdidumm]

If I try this fix out locally it changes nothing, the warning still appears - which makes sense IMO because the validation isn't depending on whether or not something is $bindable(). Furthermore $bindable() only makes the fallback value deeply reactive, not the value from the parent, and in this case the parent (client.js) always sets this value (which makes me wonder why we even set a fallback value).
The fix is to either silence the warning in this case (treat bind:this as a special case) or add a way to silence warnings in the Svelte compiler, because in this case it's a false positive.

[paoloricciuti]

If I try this fix out locally it changes nothing, the warning still appears - which makes sense IMO because the validation isn't depending on whether or not something is $bindable(). Furthermore $bindable() only makes the fallback value deeply reactive, not the value from the parent,

Uh weird it was working for me...

My reasoning was that this PR sveltejs/svelte#12484 made so that non-bindable props are not reactive even if there's a default value so i thought that adding $bindable could solve the issue.

and in this case the parent (client.js) always sets this value (which makes me wonder why we even set a fallback value). The fix is to either silence the warning in this case (treat bind:this as a special case) or add a way to silence warnings in the Svelte compiler, because in this case it's a false positive.

Ok so basically we don't need this to be state because we are not actually reacting to it, we are just using bind:this to update the array to be used to reset the scroll position and stuff right? Maybe we could just add the ignore comment in the generated code?

[dummdidumm]

There's currently no way to ignore runtime warnings - we would need to add that capability

[paoloricciuti]

There's currently no way to ignore runtime warnings - we would need to add that capability

Ohhh right it's a runtime warning. IMHO while the warning is fine there cases to bind to a non reactive variable (if I just want to use that variable only in JS and not react to it). I'll try to take a look later but I think at this point this can be closed since most likely we need to do something in svelte

[paoloricciuti]

I mean in this specific case we could just avoid transforming validate_bindings is there's a comment before. But maybe we should find a more generic mute runtime warning solutions?

[paoloricciuti]

I mean in this specific case we could just avoid transforming validate_bindings is there's a comment before. But maybe we should find a more generic mute runtime warning solutions?

@dummdidumm i got this working for this specific warning...do you think is worth a PR or should i find a more general approach?

[paoloricciuti]

I mean in this specific case we could just avoid transforming validate_bindings is there's a comment before. But maybe we should find a more generic mute runtime warning solutions?

@dummdidumm i got this working for this specific warning...do you think is worth a PR or should i find a more general approach?

I mean it was already implemented and we can always close so....why not 😄

sveltejs/svelte#12608

[dummdidumm]

Closing in favor of the referenced issue/PR