Skip to content

chore: always populate batch_values when updating a derived#17909

Open
Rich-Harris wants to merge 1 commit intomainfrom
always-populate-batch-values
Open

chore: always populate batch_values when updating a derived#17909
Rich-Harris wants to merge 1 commit intomainfrom
always-populate-batch-values

Conversation

@Rich-Harris
Copy link
Member

Never really understood this code if I'm honest. It was necessary at one point (it was introduced in #17105, and if you rewind the repo to that PR and remove the if guard, tests fail) but it doesn't seem to be doing anything now, possibly as a result of the scheduling refactor.

And it's contributing to some undesirable behaviour — if we only populate batch_values when we're reading a derived while an effect is running, then we don't populate batch_values when we call update_derived inside is_dirty. In practice what this means is that we execute the derived once when we're checking to see if an effect that depends on it needs to re-run, and then again when we re-run the effect. That's silly!

This PR doesn't fix that by itself. We additionally need is_dirty(d) to return false if batch_values.has(d). But we can't do that yet, because in some circumstances it will lead to a derived incorrectly remaining DIRTY. Needs further investigation, either in this PR or a follow-up.

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

@changeset-bot
Copy link

changeset-bot bot commented Mar 12, 2026

⚠️ No Changeset found

Latest commit: fda92ec

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

@github-actions
Copy link
Contributor

Playground

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

@dummdidumm
Copy link
Member

dummdidumm commented Mar 12, 2026

mhhhm something tells me this is wrong; my fear is that we will get an undeleteable batch_values state. But I can't reproduce that, so maybe not?
It works unconditionally since #17362, so that PR fixed it differently somehow (asked copilot to do a git bisect in #17912)

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