[fix] propagate bindings correctly

[dummdidumm]

Fixes #8103 introduced through #7981
Keeps the infinite loop from happening but reopens #6298 and #5689, as I currently see no good way to keep unnecessary bindings from firing while making sure it's fired at all times where it's needed (also see my comment further below).

Original PR description, now obsolete:

Fixes #8103 introduced through #7981 while keeping the referenced bug in that PR fixed. This expands the "are the values equal"-check from just member expressions (bind:foo={member.expression}) to all occasions, because in loop scenarios the ctx object is temporarily lagging behind when doing the $$invalidate call.

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

• Run the tests with npm test and lint the project with npm run lint

[baseballyama]

I think the test actually failed.

[dummdidumm]

Fuck, yes it's valid that this fails. I tested it with === locally but that's not what safe_not_equal does. And using === breaks other tests. I'm not sure at this point how to fix both bugs at the same time ..

[dummdidumm]

Writing down my findings for future readers, to reach a implementation that is probably not perfect but results in the least amount of weird behavior/bugs.

Why the fix I did doesn't work

Just checking if the prop that is bound has an undefined value defined in the parent doesn't work because

• this fails when the property that is bound to is a non-prop (export const/function etc), these should always be bound in the parent
• this fails when the property is reassigned eaglery during construction of the child component (for example export let foo; foo = 1) (this kind of eager reassignment could also happen in the template, or indirectly through eager function calls!)

Possible fixes and why some of them don't work

What these fixes all have in common is that they call the code that runs the binding update from the child component, not from the parent component, after a certain condition has been met. For export const/function we can analyze this statically and say "these things are not props, they have to call their parent's bindings either way" and put a corresponding statement in the generated code. The drawback is that this creates more code, and often unnecesarily because that export might not even be bound.

For the expot let .. props, these are the fixes I thought of:

Statically analyze eager reassignments

... and then put a corresponding "call these binding functions" code in the component. The problem here is that we can't statically analyze that a prop is eagerly reassigned, since that could happen in a function, which is passed to some function outside the component, which calls this function (and in other cases it would be super hard to follow the chain of calls). So the imperfect solution would be "call this binding if we find any reassignment to that binding anywhere", which will result in a lot of false positives (similar to the previous solution), probably almost as many, because why would you do bind:x when x never updates inside the child.

Some kind of Proxy stuff

This would add performance overhead and also probably go against the whole Svelte compiler design, and is a significant undertaking.

Add runtime dirty check

We already have the dirty bitmask at our hands - we could check if there's anything marked dirty after the setup. But the dirty array isn't used yet at this point, it's only afterwards that it's used. Furthermode, eager updates like export let foo = ''; foo='b' don't compile to invalidate calls currently, so that would have to be added, which probably has all kinds of other unintended side-effects.

Add a different runtime check

Basically a variation of the dirty check, but wrapping all eager updates with some kind of special invalidate, and then call bind only for those who are dirty. This probably adds some code overhead, but likely the only thing that could actually work.

Will this reintroduce the infinite loop that was fixed through that flawed PR?

I haven't checked yet, but if we implement the last of the four possible fixes it very well could. Probably the best way forward for now is to revert the PR, and look at a different way to break the loop. Then at least that PR stays fixed, but #5689 and #6298 would have to be reopened.

[dummdidumm]

I updated the PR with a fix that reopens two issues related to bindings, but keeps the infinite loop from happening and also closes the arguably worse bug that the previous PR introduced.

[shirotech]

Could this please be merged soon? I can't use any of the svelte versions as of now:

3.53.1 and earlier (repl: https://stackblitz.com/edit/vitejs-vite-izvo9q?file=src%2FApp.svelte&terminal=dev) has issues with some of the bindings causing things to be undefined, 3.54.0 and above has the issue mentioned in this PR.

[lukaszpolowczyk]

I am also waiting for this fix.

[baseballyama]

Can you tell an example that comes to the catch statement?
Is it intended to handle Maximum call stack size exceeded?

Is there any possibility that this may cause the unintended DOM structure and the runtime to go wrong behavior?

[dummdidumm]

There's on test that throws a runtime error (I believe some dev time check for svelte:element) and then all other tests afterwards fail because flush is in a broken state then. Yes, we could end up in a broken Dom state but at least the app might continue to work. The alternative would be to not do this but only do console.error in that check.

[baseballyama]

I tried to find a better way but I couldn't find a way.

This PR should make things better since #8103 could be fixed.
And #6298 could be considered a "conservative reactivity" rather than a bug.
Also, I have checked that #7032 has been fixed by this PR.

Therefore, I am merging this PR.
If someone has a better idea, please create a new PR!

[probablykasper]

Dumb question: Why is there an invalidation check at all instead of just initializing the bound variables with the final value?

[baseballyama]

According to my understanding, as a simple explanation, the Svelte compiler does not have a linker-like feature that allows optimization between components. For instance, when you compile component X, you do not know about component Y.
To achieve this, we would need additional processes at the runtime, but I don't think there is consensus that performance degradation should be accepted for this issue.

[Prinzhorn]

Thanks, that explains why fixing it is so hard. I wasn't expecting it to be such an issue. So correct me if I'm wrong: you have component A and a nested component B. A uses B and binds to a prop. Now the compiler injects the invalidation code into A and not B (which I was expecting)? Because if not then I don't quite understand why the generated code for B can't be changed to not invalidate in certain cases. In other words bind:foo={bar} causes the (unnecessary?) invalidation to be injected and not the export let foo; part? But then how does foo = 1; in the nested component travel back up?

Edit: Ok, maybe I get it. Because the "conditional invalidation" inside B would ultimately require knowledge about whether A does foo={bar} vs bind:foo={bar}, right?

I might sooner or later dive into Svelte myself, I'm curious how all these things work. For now thanks for looking into it!