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

Svelte 5: Missing warning for non-state use in $effect? #12142

Open
brunnerh opened this issue Jun 22, 2024 · 5 comments
Open

Svelte 5: Missing warning for non-state use in $effect? #12142

brunnerh opened this issue Jun 22, 2024 · 5 comments

Comments

@brunnerh
Copy link
Member

Describe the bug

When a non-state value that is assigned is used in the template, you get a warning:

value is updated, but is not declared with $state(...). Changing its value will not correctly trigger updates (non_reactive_update)

Could/should such a warning be shown if the value is only used in an $effect?

(Related/found via: #12141)

Reproduction

<script>
  import { onMount } from 'svelte';

  let value;
	
  $effect(() => {
    console.log(value);
  });

  onMount(() => {
    value = 1;
  });
</script>

REPL

Logs

No response

System Info

REPL

Severity

annoyance

@FoHoOV
Copy link
Contributor

FoHoOV commented Jun 22, 2024

If it is decided to have warning, it shoudn't be for usages of none-reactive values in effects/deriveds. IMHO a warning could be there, only if there are 0 reactive values inside an effect/derived. For example an effect could depend on only 1 reactive value, but use 10 other variables that are not reactive which is fine. If nothing in the effect, triggers a rerun, then there could be a warning. Also one might use $effect just for onMount and then cleanup logic. Thoughts?

@MotionlessTrain
Copy link
Contributor

If it is decided to have warning, it shoudn't be for usages of none-reactive values in effects/deriveds

The warning is to make sure you don't forget assigning them with $state, if they were meant to be reactive. That is also why it is a warning, and not an error, as it is fine to use those if you don't need the reactive dependency

@brunnerh
Copy link
Member Author

The question is whether this would lead to many false positives or not.
To evaluate that, testing the warning on real world code would probably be helpful.

@paoloricciuti
Copy link
Member

The question is whether this would lead to many false positives or not. To evaluate that, testing the warning on real world code would probably be helpful.

I think i'm with @FoHoOV on this one...there are reasons to use non stateful variables in an effect. The error should really just be thrown if that's the only dependency but this also means that the first function call should mute this error. Maybe even property access.

@brunnerh
Copy link
Member Author

I suspect you both overestimate how often the warning would trigger - it is very specific.

The variable has to be...

  • non-reactive
  • locally declared
  • locally modified

E.g. constants and imports would not be affected.

How often do you really want to change a local value, use it in an $effect and not have the effect be triggered by that change?

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

4 participants