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

feat: allow state created in deriveds/effects to be written/read locally without self-invalidation #15553

Merged
merged 23 commits into from
Mar 20, 2025

Conversation

Rich-Harris
Copy link
Member

This implements the idea in #15300 (comment). It means that state created inside a reaction (i.e. a derived or effect) can be written and read inside that reaction without causing it to re-run.

This means that things like this, which fail on main, because a simple console.log(this.name) causes an 'unsafe read'...

class User {
  name = $state();
  
  constructor(name) {
    this.name = name;
    console.log(this.name);
  }
}

const user = $derived(new User(name));

...work totally fine in this PR.

I don't see any real downside to this, and I don't see it as a breaking change (since the changed behaviour only concerns code that would have previously failed altogether). The only slight trickery concerns proxies — sources that are created lazily as properties are read need to have the same parent as the original $state, otherwise in a situation like this, which should cause an infinite loop...

let object = $state({ count: 0 });

$effect(() => {
  object.count += 1;
});

...no infinite loop would occur, because object.count would 'belong' to the effect. Userland does not have this ability, so anyone implementing their own lazy state mechanism would potentially find that assignments that 'should' cause an infinite loop don't. Personally I'm very okay with this.

Draft because it could probably use a few tests; will try and get to that tomorrow.

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

  • 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

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…action

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…ts with $.derived

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link

changeset-bot bot commented Mar 20, 2025

🦋 Changeset detected

Latest commit: 82c6f39

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Minor

Not sure what this means? Click here to learn what changesets are.

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

@svelte-docs-bot
Copy link

Copy link
Contributor

Playground

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@Rich-Harris

This comment was marked as resolved.

@Rich-Harris Rich-Harris marked this pull request as ready for review March 20, 2025 16:14
Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I wonder if there might be any unforeseen issues with keeping a round a reference to the parent on state signals. Like if you create a proxied object in an effect with a ton of properties and then pass that to something else outside the effect. That effect will never be GC'd even if it's unmounted because all those state signals will retain that reference forever.

@Rich-Harris
Copy link
Member Author

Good catch — addressed that by reinstating a version of the derived_sources trick, but with the distinction that we only append to it if the effect that owns the source is currently updating. This also fixed a notable performance regression with the previous approach

@Rich-Harris Rich-Harris merged commit 6915c12 into main Mar 20, 2025
13 checks passed
@Rich-Harris Rich-Harris deleted the untrack-own-sources branch March 20, 2025 20:04
@github-actions github-actions bot mentioned this pull request Mar 20, 2025
hopperelec added a commit to hopperelec/metro-tag-manager that referenced this pull request Mar 20, 2025
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 this pull request may close these issues.

None yet

2 participants