Skip to content

chore: simplify source ownership #16333

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 5 commits into from
Jul 10, 2025
Merged

chore: simplify source ownership #16333

merged 5 commits into from
Jul 10, 2025

Conversation

Rich-Harris
Copy link
Member

I have a feeling we could make this simpler still, by comparing the update_version with the value at the time when the signal was created, but I haven't yet figured out an approach that doesn't result in an additional property on the signal (I thought we could use rv, but it makes it harder to avoid duplicate dependencies).

In the meantime, we don't need to create an object with a reaction property in addition to the sources; as long as we update source_ownership (here renamed to current_sources) when we need to lazily create sources inside proxy.js. Previously we were using set_active_reaction as a way to achieve the same outcome, but in a more roundabout way (normally that function is used so that the effect tree is constructed correctly, but that doesn't apply here).

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

Copy link
Contributor

Playground

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

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

This means we keep around an array of sources for longer vs one reaction previously. Hard to say what keeps more memory, I assume it's largely the same, so 👍

@Rich-Harris
Copy link
Member Author

That's true... an alternative would be to prevent push_reaction_sources from running if the child source is being created after the initial reaction. I tried to make that work but it was finicky. If I'd realized we were hanging onto the array for longer with this change I'd have kept plugging away at it

Copy link

changeset-bot bot commented Jul 10, 2025

🦋 Changeset detected

Latest commit: 85e7439

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
Copy link
Member Author

Changed the implementation so that current_sources can be GC'd at the end of update_reaction

@Rich-Harris Rich-Harris merged commit 6f0aec5 into main Jul 10, 2025
14 checks passed
@Rich-Harris Rich-Harris deleted the simplify-source-ownership branch July 10, 2025 16:36
@github-actions github-actions bot mentioned this pull request Jul 10, 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.

2 participants