Skip to content

make internal sources ownerless #13013

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

Merged
merged 10 commits into from
Aug 24, 2024
Merged

make internal sources ownerless #13013

merged 10 commits into from
Aug 24, 2024

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Aug 24, 2024

WIP fix for #13011. Essentially, all the sources that we create internally (version sources on proxies and reactive sets/maps/etc) need to be ownerless to avoid triggering the self-dependency guard.

One way to do that is to add , null to all the internal source call-sites, which is what I've started doing here. But really, that's backwards — the real (and future-proof) fix is to have an internal representation of $state that is distinct from source. Working on that now

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.

Tests and linting

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

Copy link

changeset-bot bot commented Aug 24, 2024

🦋 Changeset detected

Latest commit: 2db64e0

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

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

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

@Rich-Harris Rich-Harris marked this pull request as ready for review August 24, 2024 21:13
@Rich-Harris
Copy link
Member Author

One thing I notice: in the case of a state proxy that does not have a top-level source, we don't trigger the guard logic — for example if we tweak the example in #10308

<script>
  let elapsed = $derived.by(() => {
    let object = $state({ seconds: 0 });

    setInterval(() => {
      console.log('incrementing');
      object.seconds += 1;
    }, 1000);

    return object.seconds;
  });
</script>

<p>{elapsed}</p> 

That's true on main as well so I don't think it needs to be fixed as part of this PR, but making a note of it. I guess we'd need to track the owner inside the top-level $.proxy(...) call and apply similar logic inside the get handler or something

@Rich-Harris Rich-Harris merged commit ae27f27 into main Aug 24, 2024
9 checks passed
@Rich-Harris Rich-Harris deleted the gh-13011 branch August 24, 2024 21:36
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.

1 participant