fix: don't re-attach the same function to the same element

[paoloricciuti]

Closes #16044

As I've said on the issue I think it will have a performance impact both because we need to call Object.getOwnPropertySymbols on the previous values, find the function in it (and we need to do that for every attachment) and we also need to call delete effects[that_symbol] (I know delete is veeeeery slow)...that said we should gain by not recreating an effect so maybe worth it unnecessarily?

However it also just occurred to me that this is technically a breaking change since someone might rely on this behaviour? Also imagine a situation like this

function the_actual_attachment(node){
    // do stuff
}

function another_actual_attachment(node){
    // do stuff
}

function attachment_factory(arg){
    if(arg % 2 === 0) return the_actual_attachment;
    return another_actual_attachment;
}

this will basically not rerun if you switch the argument from 2 to 4.

So yeah I'm a bit contrived about this changes but I implemented it to see if it's possible so maybe let's discuss it.

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]

🦋 Changeset detected

Latest commit: 267339e

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]

Playground

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

[Rich-Harris]

This feels like user error. Why are we calling createAttachmentKey inside the derived?

[Rich-Harris]

(Maybe a dev time check would be warranted?)

[paoloricciuti]

This feels like user error. Why are we calling createAttachmentKey inside the derived?

Yeah the alternative would be to document this because it might not be obvious to everyone that it's returning a different symbol.

But the question was "If the function is the same, should we be able to not re attach it" which I'm a bit thorned if that should be the case. Is there a case where the same function would be attached twice to the same element intentionally?

[CaptainCodeman]

This feels like user error. Why are we calling createAttachmentKey inside the derived?

It still does it if createAttachmentKey is called outside it:
https://svelte.dev/playground/542b5db158e148feafde1451c6cbd900?version=5.33.10

So it's more than just the symbol - it needs the entire action fn to be external too (which wasn't intuitive).

[Rich-Harris]

That's definitely expected behaviour — if it's the same symbol but a different function each time then Svelte can't know that it's supposed to treat it as the same function just because it 'looks' the same.

As to whether same-function-different-key should be stable... I don't think that would be considered a breaking change and I don't think there'd be anything wrong about preserving it across runs. Just want to make sure we don't overcomplicate stuff for an edge case

[paoloricciuti]

It still does it if createAttachmentKey is called outside it: https://svelte.dev/playground/542b5db158e148feafde1451c6cbd900?version=5.33.10

So it's more than just the symbol - it needs the entire action fn to be external too (which wasn't intuitive).

Yeah there's not much we can do if the function itself changes, actually it needs to rerun if the function changes or it will break stuff

Just want to make sure we don't overcomplicate stuff for an edge case

I mean it is kinda of an edge case, that's why I'm torn about it...do you think it can have performance implications?

[CaptainCodeman]

Yeah there's not much we can do if the function itself changes

I thought it only updated what changed, I guess not.

[paoloricciuti]

I thought it only updated what changed, I guess not

If you recreate the function in the derived that's a different function everytime

[CaptainCodeman]

I was thinking it was fine-grained like $state but the whole thing is replaced, so changes fire for props that haven't changed (but then not for deriveds of those props).

https://svelte.dev/playground/db00928094cf49d98831f44a2c62bd7c?version=5.33.11

[Rich-Harris]

It is fine-grained. You're creating a new function inside the derived expression. It sounds like you want to do this?

[CaptainCodeman]

Yes, that's the conclusion I came to in the original issue

[Rich-Harris]

I'm going to go ahead and close this then unless it proves to be a consistent source of confusion. If anything, we want to discourage people from creating new attachment keys all the time anyway, since that's wasteful