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

SvelteMap reactivity not fully fixed by #13877 #13947

Closed
gyzerok opened this issue Oct 26, 2024 · 6 comments · Fixed by #13951
Closed

SvelteMap reactivity not fully fixed by #13877 #13947

gyzerok opened this issue Oct 26, 2024 · 6 comments · Fixed by #13951

Comments

@gyzerok
Copy link

gyzerok commented Oct 26, 2024

Describe the bug

In #13877 there was a fix for SvelteMap, which fixed the original reported REPL, but was still a problem in our codebase. It seems I found what the issue is.

@trueadm fyi

Reproduction

Here is the REPL.

The difference is calling getAsync the second time in {@const value1 = cache.getAsync(id)}. This again ends up in the infinite loading state. In our case we are probably calling from some other place, but here I place it on the next line for the minimal repro purpose.

Relevant piece from the playground inline for your convenience

{#await id}
  Loading...
{:then id}
  {@const value = cache.getAsync(id)}
  {@const value1 = cache.getAsync(id)}

  {#if value instanceof Promise}
    Loading...
  {:else}
    {value}
  {/if}
{/await}

Logs

No response

System Info

irrelevant

Severity

annoyance

@gyzerok gyzerok changed the title SvelteMap reactivity not fully fixed by 13877 SvelteMap reactivity not fully fixed by #13877 Oct 26, 2024
@paoloricciuti
Copy link
Member

This was super weird to debug...i kinda fixed it but then i went to write the test and it was not failing even when removing the fix...so i discovered that this is a behaviour that is specifically related to dev and const tag (i'm trying to understand if there could also be other situations where it's problematic).

The reason it doesn't work is because const values are immediately read in dev so when it comes to setting the map it already has a reaction and the fix in #13877 doesn't work. Let me investigate further.

@gyzerok
Copy link
Author

gyzerok commented Oct 26, 2024

I'll check if in our project it's only a problem in dev. Will report later. Thank you for your investigation!

@gyzerok
Copy link
Author

gyzerok commented Oct 26, 2024

In our code the problem still exists even in the prod build. So either a different reproduce is needed or it's not a dev-only problem. I can try to check our code with version from your PR to see if it helps. Does it contain all the necessary pieces already @paoloricciuti?

@paoloricciuti
Copy link
Member

I'll check if in our project it's only a problem in dev. Will report later. Thank you for your investigation!

Technically it could not be a problem only in dev is a both derived are read before the set in some way...my pr should fix this so please check if it does (even tho i'm not quite convinced is the right fix)

@gyzerok
Copy link
Author

gyzerok commented Oct 27, 2024

Oh, unfortunately this still does not fix the issue in my project (sigh). Will continue looking for the right repro.

Anyway, your fix left even less room for the issues to appear. Thank you!

@paoloricciuti
Copy link
Member

Mmm I really wonder what's going on

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

Successfully merging a pull request may close this issue.

2 participants