Skip to content

Conversation

@xxkl1
Copy link
Contributor

@xxkl1 xxkl1 commented Mar 21, 2023

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

fix: #8305

The reason of this issue is key block bind's callback will make dirty, and object always safe_not_equal .

So key block will recreate when component bind's callback run, and then binding_callbacks add new callback.

binding_callbacks's length always > 0, can't get out of while loop.

This example can freezes page too.
Can paste it into svelte's repl and observe the reaction.

The solution is run component bind proactively when create a key block only at the first time.

Component bind value use parent value when next key block recreate, don't run callback proactively.

@vercel
Copy link

vercel bot commented Mar 21, 2023

@xxkl1 is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@dummdidumm
Copy link
Member

I don't think we can just omit rerunning the binding if it's inside a key block

  • we don't know if the binding and the keyed variable have a relationship, there's not always an infinite loop, so it could be right to update the bound variable
  • I haven't confirmed this, but from looking at the code it seems this assigns a variable to the function block, so this will stop any binding from updating after a key has updated once

@xxkl1
Copy link
Contributor Author

xxkl1 commented Mar 21, 2023

I don't think we can just omit rerunning the binding if it's inside a key block

  • we don't know if the binding and the keyed variable have a relationship, there's not always an infinite loop, so it could be right to update the bound variable
  • I haven't confirmed this, but from looking at the code it seems this assigns a variable to the function block, so this will stop any binding from updating after a key has updated once

The infinite loop problem occurs in the processing of binding_callbacks at flush.

        while (binding_callbacks.length)
            binding_callbacks.pop()();

binding_callbacks.pop()() will update parent value, and then key block recreate.

key block recreating push new binding_callback to binding_callbacks.

binding_callbacks.length can't become zero and can't leave while loop.

So I think don't actively update the value of parent in the key block second create

Then binding_callbacks.length can become zero and leave while loop.

Component bind value use parent value when key block second create.

I make a called flag to key block create function.

And don't update parent value when key block create function is called.

@xxkl1
Copy link
Contributor Author

xxkl1 commented Mar 21, 2023

this commit code generation at result:

function create_key_block(ctx) {
	binding_callbacks.push(() => bind(mycomponent, 'week', mycomponent_week_binding, !create_key_block._called));
	create_key_block._called = true;
}
function bind(component, name, callback, runCallback = true) {
    const index = component.$$.props[name];
    if (index !== undefined) {
        component.$$.bound[index] = callback;
        if (runCallback) {
            callback(component.$$.ctx[index]);
        }
    }
}

make callback(component.$$.ctx[index]); run only at create_key_block first call.

parent value will be updated only key block inside, but not be updated when key block update.

Updating parent value is seem incorrect when key block update.

@dummdidumm dummdidumm added this to the one day milestone Mar 22, 2023
@dummdidumm
Copy link
Member

Yeah the flush logic is prone to infinite loop. My opinion is that we can't get away with fixing it like this. I think we need to generally overhaul some of this stuff, though I'm not sure how that could look like yet.

@xxkl1
Copy link
Contributor Author

xxkl1 commented Mar 22, 2023

Yeah the flush logic is prone to infinite loop. My opinion is that we can't get away with fixing it like this. I think we need to generally overhaul some of this stuff, though I'm not sure how that could look like yet.

I think so, need to find the right way to handle it.

@xxkl1 xxkl1 closed this Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Key block freezes website

2 participants