Skip to content

Conversation

@Rich-Harris
Copy link
Member

Currently, an Effect has three properties that default to nullnodes_start, nodes_end, and transitions — and which are only used if the effect is a branch effect (or html/head effect, but you get the idea).

Since smaller objects are faster to work with and take up less memory, it's almost certainly preferable to reduce that to a single property, nodes, which is populated if it's the sort of effect that contains DOM.

My real motivation is this, though: I'm working through the edge cases in #17258 and it's hellishly complicated, made doubly so by the fact that every each block has two linked lists that resemble each other but don't fully match — a list of 'items' and a list of their effects. We only really need the second of these. Getting rid of the items list will reduce memory usage, and reduce the amount of work that needs to happen during reconciliation. But to get there, we need to move the AnimationManager from EachItem and onto Effect. If it became a direct property of Effect that would add unnecessary overhead (animations are exceedingly rare, relative to effects as a whole). But if it's part of nodes, it's fine. Hence the larger change.

(It's possible that by putting animations on effects, we could eventually relax the restriction that says the animate: directive has to be a direct child of a keyed each block, though that's a separate conversation that will probably be overtaken by a broader rethink of how animations and transitions work.)

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 Dec 2, 2025

🦋 Changeset detected

Latest commit: bf195b5

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

Playground

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

@Rich-Harris
Copy link
Member Author

gonna go ahead and self-merge this to unblock further work, otherwise it will become very hairy to juggle everything

@Rich-Harris Rich-Harris merged commit 1f3f182 into main Dec 2, 2025
18 checks passed
@Rich-Harris Rich-Harris deleted the effect-nodes branch December 2, 2025 21:53
@github-actions github-actions bot mentioned this pull request Dec 2, 2025
@JonathonRP
Copy link

I just wanted to up vote the rethinking of animations, and I do like the idea of lifting animations restriction to just each block. I think a lot of us in animations have been wanting it but not noisy about it 😅

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.

3 participants