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

Common auto-save pattern does not work for inputs #14911

Open
MathiasWP opened this issue Jan 6, 2025 · 12 comments
Open

Common auto-save pattern does not work for inputs #14911

MathiasWP opened this issue Jan 6, 2025 · 12 comments

Comments

@MathiasWP
Copy link
Contributor

MathiasWP commented Jan 6, 2025

Describe the bug

Using onblur for autosaving inputs is a common pattern, and it seems that if the save-callback takes more than 100ms then Svelte does not behave as expected.

Related issue: sveltejs/kit#13276

This bug is possible to work around, but it really messes with our mental model of how props/state work. Also note that while setting up the repo it behaved flaky. Sometimes i was not able to trigger the bug at all, while other times it would work with just Promise.resolve() (using no timeout at all to mimic a request). Not sure if this is because of HMR 🤷

Reproduction

Minimal repro: https://svelte.dev/playground/2b6ba5779ffe4ae2ac2e5f493d1d14e0?version=5.16.2

Screen.Recording.2025-01-06.at.13.09.48.mov

Logs

No response

System Info

System:
    OS: macOS 15.2
    CPU: (8) arm64 Apple M1 Pro
    Memory: 100.27 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.0 - ~/.nvm/versions/node/v20.11.0/bin/node
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.0/bin/npm
    pnpm: 9.13.2 - /opt/homebrew/bin/pnpm
    bun: 1.0.0 - ~/.bun/bin/bun
  Browsers:
    Brave Browser: 131.1.73.104
    Chrome: 131.0.6778.205
    Edge: 131.0.2903.112
    Safari: 18.2
  npmPackages:
    svelte: ^5.0.0 => 5.16.2

Severity

annoyance

@Thiagolino8
Copy link

This behavior is expected, the blur event simply takes longer to reference the object than the click event takes to turning it null

If you use onmousedown instead of onclick you don't even need to simulate waiting

It's not a bug, it's the correct behavior

To fix it, just grab the reference before waiting

@MathiasWP
Copy link
Contributor Author

MathiasWP commented Jan 6, 2025

This behavior is expected, the blur event simply takes longer to reference the object than the click event takes to turning it null

If you use onmousedown instead of onclick you don't even need to simulate waiting

It's not a bug, it's the correct behavior

To fix it, just grab the reference before waiting

I'm not sure if this is the case. The blur is supposed to fire before the click, because there is no capture defined on the click. It's a bit weird hack to have to create a new reference, because it shouldn't really be necessary. Here's the same component written in React, and there's no issue with the state being undefined in this case: https://playcode.io/2212977

Screen.Recording.2025-01-06.at.15.27.21.mov

This can also be triggered by other things than click, for example client side navigation. If you perform a client side navigation, and the prop is derived by some data + route params, then the source may change, causing similar undefined bugs.

@Thiagolino8
Copy link

I'm not sure if this is the case. The blur is supposed to fire before the click

It fires before, but because of the await the console.log line only runs after

React behaves differently because the value of a state only updates on the next render

That's why:

const [count, setCount] = useState(0)
console.log(count) // logs 0
setCount(1)
console.log(count) // logs 0

you can reference the previous value not because it is the correct behavior, but because of a stale closure

@MathiasWP
Copy link
Contributor Author

I'm not sure if this is the case. The blur is supposed to fire before the click

It fires before, but because of the await the console.log line only runs after

React behaves differently because the value of a state only updates on the next render

That's why:

const [count, setCount] = useState(0)
console.log(count) // logs 0
setCount(1)
console.log(count) // logs 0

you can reference the previous value not because it is the correct behavior, but because of a stale closure

I see! But shouldn't Svelte keep the closure so this doesn't happen?

@Thiagolino8
Copy link

Not really

I understand where your expectation comes from, coming from react

But I don't think that's most people's expectation, that's not how javascript works

https://codepen.io/thiagolino8/pen/VYZyPrz?editors=1111
image

@MathiasWP
Copy link
Contributor Author

MathiasWP commented Jan 6, 2025

Not really

I understand where your expectation comes from, coming from react

But I don't think that's most people's expectation, that's not how javascript works

codepen.io/thiagolino8/pen/VYZyPrz?editors=1111 image

This makes sense, but i think it was a little bit difficult to wrap our heads around this behaviour since it worked fine in Svelte 4: https://svelte.dev/playground/2b6ba5779ffe4ae2ac2e5f493d1d14e0?version=4.0.5

I've always felt that Svelte gives you superpowers when combining state and the DOM, and this is a case where it feels like Svelte 5 does not fill this gap.

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Jan 6, 2025

This is somewhat related to #14707

TBH with code like:

{#if data}
  <Input item={data.item} />
{/if}

I would assume that the item prop could never become undefined inside Input (goes against how if-blocks are supposed to behave). But apparently in #14707 people seem to agree that this is ok.

But at the same time, Svelte cannot know (?) that you are holding a reference to item and cleans everything up. Similar to #11424 Svelte would need to be able to track state in a async context, which browser JS engines simply lack the tools for.

@Prinzhorn
Copy link
Contributor

Maybe Svelte needs a

Tried to access state/props after unmounting

error similar to React when you're updating state in an unmounted component? This would make this a lot clearer.

@Thiagolino8
Copy link

but i think it was a little bit difficult to wrap our heads around this behaviour since it worked fine in Svelte 4

Svelte 4, like React, had a render cycle (although infinitely more efficient), but in the end it was also a matter of synchronization
Svelte 5 uses getters so that there is no need for prop synchronization, you receive the data directly from the source

I would assume that the item prop could never become undefined inside Input

And it really can't in synchronous code, but race conditions are something you have to learn to deal with in asynchronous javascript code.

error similar to React when you're updating state in an unmounted component? This would make this a lot clearer

I can see this working

@MathiasWP
Copy link
Contributor Author

MathiasWP commented Jan 6, 2025

This is somewhat related to #14707

TBH with code like:

{#if data}
  <Input item={data.item} />
{/if}

I would assume that the item prop could never become undefined inside Input (goes against how if-blocks are supposed to behave). But apparently in #14707 people seem to agree that this is ok.

But at the same time, Svelte cannot know (?) that you are holding a reference to item and cleans everything up. Similar to #11424 Svelte would need to be able to track state in a async context, which browser JS engines simply lack the tools for.

I've also felt numerous time that this goes against how if-blocks are supposed to behave. It's not even possible to catch with TypeScript, so these cases are really difficult to spot.

Also, do not get too hung up in the simple reproduction i've provided - i tried creating a minimal repro to showcase the issue. I've met a lot tougher cases in our enterprise app where we are surprised by why it doesn't work, and it's always a tiny piece of code that in some unexpected way references a value that has become undefined. It's really really difficult to trace these code-paths, because they aren't visible in any way.

It would be really nice if Svelte gave a helping hand in these cases, or if there was a way to make TypeScript actually know that it can be undefined automatically.

This becomes a case where you "just have to know" that this won't work. Code that behaves like this is prone to creating bugs (cough Vanilla JS cough).

@Thiagolino8
Copy link

Thiagolino8 commented Jan 8, 2025

For the record, vue and solid have the same behavior

@MathiasWP
Copy link
Contributor Author

For the record, vue and solid have the same behavior

Nice to know! And thinking more about this it does make sense why it happens - but there's still something that feels wrong about this case not being easily caught when using TypeScript :/

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

No branches or pull requests

3 participants