Skip to content

fix: reuse already attached attachments in spread attributes #15951

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Ocean-OS
Copy link
Contributor

Fixes #15949, needs a test before being merged

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

changeset-bot bot commented May 18, 2025

🦋 Changeset detected

Latest commit: 2c45b7e

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

Copy link
Contributor

Playground

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

@paoloricciuti
Copy link
Member

I was fixing the same thing: this doesn't work because the effect created by attach is still destroyed by the parent template_effect...this means the attachment is detached and never reattached.

Imho we should have a separate $.attach_attachments function that creates an effect, reads only the symbols so it's only the attachments symbols so it's only triggered if any of those changes and then attach them

That would still trigger when any attachment change tho 🤔

So I think it's a bit annoying

@Ocean-OS
Copy link
Contributor Author

Ocean-OS commented May 18, 2025

I was fixing the same thing: this doesn't work because the effect created by attach is still destroyed by the parent template_effect...this means the attachment is detached and never reattached.

Imho we should have a separate $.attach_attachments function that creates an effect, reads only the symbols so it's only the attachments symbols so it's only triggered if any of those changes and then attach them

That would still trigger when any attachment change tho 🤔

So I think it's a bit annoying

I just realized that, so I'm looking into trying to make an attachment_effect that would make a derived from the attachment getter, and then only teardown if the derived has changed (or when any parent effects are permanently torn down), not when any parent effects have rerun.
I'm not sure how functional/feasible this would be, or if it's the best solution, but I'll try it out.
...And after looking into this more, it seems less and less feasible, so I'm going to try to find another way...

@paoloricciuti
Copy link
Member

Mmm not clear if that would have any problem but I guess you can try

@Rich-Harris
Copy link
Member

#15951

@Ocean-OS
Copy link
Contributor Author

Since I'm fairly certain you meant to reference your new PR, I'll just tag it here
#15961

@Rich-Harris
Copy link
Member

whoops! indeed

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.

Unexpected behavior when combining spread operator with createAttachmentKey
3 participants