Skip to content
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.

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

state_unsafe_mutation when an error happened inside the ":then" block #14186

Closed
MartkCz opened this issue Nov 6, 2024 · 12 comments · Fixed by #14191
Closed

state_unsafe_mutation when an error happened inside the ":then" block #14186

MartkCz opened this issue Nov 6, 2024 · 12 comments · Fixed by #14191
Assignees
Labels

Comments

@MartkCz
Copy link

MartkCz commented Nov 6, 2024

Describe the bug

It seems like Svelte gets stuck in an effect state.

Reproduction

https://svelte.dev/playground/6fbbf69c47894154bfa37c9fb83e7ac0?version=5.1.11

Type something in the text input

Logs

No response

System Info

System:
    OS: Linux 5.15 Manjaro Linux
    CPU: (12) x64 AMD Ryzen 5 5600G with Radeon Graphics
    Memory: 10.44 GB / 30.70 GB
    Container: Yes
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 22.6.0 - /usr/bin/node
    npm: 10.8.2 - /usr/bin/npm
    bun: 1.1.24 - /usr/bin/bun
  Browsers:
    Brave Browser: 128.1.69.160
  npmPackages:
    svelte: ^5.1.11 => 5.1.11

Severity

annoyance

@trueadm
Copy link
Contributor

trueadm commented Nov 6, 2024

What is the {xxx.xxx} all about? It's likely that the error is causing the problem you're seeing.

@Conduitry
Copy link
Member

I think that's intentional - the report is that Svelte doesn't recover well from runtime errors like this. Which I don't think there's a lot that can be done about until we implement error boundaries, correct?

@MartkCz
Copy link
Author

MartkCz commented Nov 6, 2024

It's just to simulate an error. I can change it to:

<script>
    let { need } = $$props;

    let promise = fetch('https://jsonplaceholder.typicode.com/todos/1');
</script>

{#await promise}
{:then value}
    {value.obj.value}
{:catch error}
    {error}
{/await}

same result. It breaks all components on the website, which isn't ideal for production.

@paoloricciuti paoloricciuti self-assigned this Nov 6, 2024
@paoloricciuti
Copy link
Member

@trueadm the problem is that after the first error that happens during rendering of the then block every change is throwing state_unsafe_mutation.

This is because of point number 1 here:

  1. inside the await block if we have restore true we set the active reaction, then we run the block effect for the then function and then we restore back...but if the then function throws we never restore it back (i already fixed this with a try finally
  2. However if you do <input oninput={(e)=>term = e.target.value} value={term} /> instead of binding value the subsequent errors are not there so i suspect there's also something funky going on with bind value

@paoloricciuti
Copy link
Member

@trueadm the problem is that after the first error that happens during rendering of the then block every change is throwing state_unsafe_mutation.

This is because of point number 1 here:

  1. inside the await block if we have restore true we set the active reaction, then we run the block effect for the then function and then we restore back...but if the then function throws we never restore it back (i already fixed this with a try finally
  2. However if you do <input oninput={(e)=>term = e.target.value} value={term} /> instead of binding value the subsequent errors are not there so i suspect there's also something funky going on with bind value

Ohhh this is because bind:value it's adding an event listener normally so it's not deferred.

@trueadm
Copy link
Contributor

trueadm commented Nov 6, 2024

Nice find!

@paoloricciuti
Copy link
Member

Nice find!

Should we change bind:value too? I think it's fine if it adds the listener normally right?

@trueadm
Copy link
Contributor

trueadm commented Nov 6, 2024

it just needs to use on internally, no?

@paoloricciuti
Copy link
Member

it just needs to use on internally, no?

Yeah exactly

@trueadm
Copy link
Contributor

trueadm commented Nov 6, 2024

@paoloricciuti Are you doing that now or should we create an issue for it?

@paoloricciuti
Copy link
Member

@paoloricciuti Are you doing that now or should we create an issue for it?

I can do it probably In a bit

@trueadm
Copy link
Contributor

trueadm commented Nov 6, 2024

I've already got a PR incoming :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants