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

[docs] update warning message for state_referenced_locally #15171

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sacrosanctic
Copy link
Contributor

closes #11883 #13079

I believe there should be better examples for closures. The current example only covers setContext('count', () => count);. But there is more than 1 way to create a closure and more than 1 place this warning can appear. For example:

<script>
  const items = 5
  let itemsPerPage = $state(5)
  let pages = $state(items / itemsPerPage)
</script>

REPL

Before submitting the PR, please make sure you do the following

If there is interest. I can create a separate issue for the above.

  • 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.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Jan 31, 2025

⚠️ No Changeset found

Latest commit: 010045f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@15171

@henrykrinkle01
Copy link

I think This state is not reactive is vaguer than State referenced in its own scope will never update. What do you mean it's not reactive? I declared it with a $state! The latter isn't entire true either. It does update in certain circumstances, not never updates. Example:

let foo = $state({ value : 0 });
let bar = $state(foo);
foo.value++;
console.log(bar.value) // 1

However, it stops updating if foo is reassigned. So I think something alone these lines is more accurate:

State referenced in its own scope won't update on reassignment. Ignore this message if it's intended. Otherwise, consider not reassign the state (only mutate) or reference it in a closure\nhttps://svelte.dev/e/state_referenced_locally`

@sacrosanctic
Copy link
Contributor Author

What do you mean it's not reactive?

That's the kind of response you want to elicit in a warning. Due to quirks in svelte/js, the code the user has written may not behave as expected. The warning is not nuanced because it was meant to be grokable and a url to the docs is provided to encourage them to explore further to ensure they have written code with behaviour they expect.

@Rich-Harris
Copy link
Member

The state is reactive, it's the reference that isn't. I think the current wording is more correct than the proposed alteration.

@sacrosanctic
Copy link
Contributor Author

This state is only read once. Did you mean to reference it inside a closure?

This is one proposed by @brunnerh.

I agree that the current version is correct. But it does not grok well.

@henrykrinkle01
Copy link

henrykrinkle01 commented Feb 4, 2025

This state reference is in the same scope as the state declaration. It won't update on reassignment. Did you mean to put it inside a closure?

How about this?

@Acccent
Copy link

Acccent commented Feb 20, 2025

Maybe I'm confused, but isn't the "non-updating" aspect... probably intentional, if using $state?
i.e.

  const items = 5
  let itemsPerPage = $state(5)
  let pages = $state(items / itemsPerPage) // will not update when itemsPerPage does

Surely that should be fine, because if I did want to keep them in sync, I would have used $derived, no?

  const items = 5
  let itemsPerPage = $state(5)
  let pages = $derived(items / itemsPerPage) // updates along with itemsPerPage, since it is "derived" from it

Every time I've seen this warning I've thought "well yeah I know it's not gonna update."

@paoloricciuti
Copy link
Member

Maybe I'm confused, but isn't the "non-updating" aspect... probably intentional, if using $state? i.e.

  const items = 5
  let itemsPerPage = $state(5)
  let pages = $state(items / itemsPerPage) // will not update when itemsPerPage does

Surely that should be fine, because if I did want to keep them in sync, I would have used $derived, no?

  const items = 5
  let itemsPerPage = $state(5)
  let pages = $derived(items / itemsPerPage) // updates along with itemsPerPage, since it is "derived" from it

Every time I've seen this warning I've thought "well yeah I know it's not gonna update."

The goal of the warning is exactly because you might not realize that you are accessing state in it's own scope and might expect to actually update when it changes.

@Acccent
Copy link

Acccent commented Feb 20, 2025

The goal of the warning is exactly because you might not realize that you are accessing state in it's own scope and might expect to actually update when it changes.

Right, but then instead of suggesting to use a closure, maybe it should nudge towards $derived. Because then you also get the extra warnings when trying to reassign. Otherwise you can have this scenario:

  // ...
  let pages = $state(() => items /  itemsPerPage) // no warning
  // later
  pages = 12 // the desynchronization that was meant to be warned against happened anyway

@paoloricciuti
Copy link
Member

The goal of the warning is exactly because you might not realize that you are accessing state in it's own scope and might expect to actually update when it changes.

Right, but then instead of suggesting to use a closure, maybe it should nudge towards $derived. Because then you also get the extra warnings when trying to reassign. Otherwise you can have this scenario:

  // ...
  let pages = $state(() => items /  itemsPerPage) // no warning
  // later
  pages = 12 // the desynchronization that was meant to be warned against happened anyway

What the closure message is hinting to is that if you want to pass a reactive value to some function you need to wrap it in a closure:

let count = $state(0);

// wrong
const counter = double_it(count);

//right
const counter = double_it(()=>count);

@Acccent
Copy link

Acccent commented Feb 20, 2025

I see, thanks for the clarification. I guess then, ideally I think there should be a different warning when the reference is itself inside a $state – and it should be disabled when there's sufficient hints that the user knows what they're doing, i.e.:

  let count = $state(0);

  const modifiedOutdated = doSomething(count);
  // warning:
  // This state reference breaks reactivity and will always be equal to the state's initial value.
  // Consider using a closure to get the updated value.

  const modifiedSnapshot = doSomething($state.snapshot(count));
  const getModified = doSomething(() => count);
  // no warning

  let modifiedState = $state(doSomething(count));
  const getModifiedState = $state(doSomething(() => count));
  // warning:
  // State referenced in another state may lead to unexpected desynchronization.
  // Consider using $derived to keep states synced or $state.snapshot to explicitly de-sync.

  let modifiedDerived = $derived(doSomething(count)); // or $derived(doSomething(() => count))
  let modifiedStateSnapshot = $state(doSomething($state.snapshot(count)));
  // no warning

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.

Svelte 5: Confusing and unclear warning state_referenced_locally
5 participants