fix: lazily create sources for Set

[paoloricciuti]

Svelte 5 rewrite

Closes #11939

i think this time i did the right thing and didn't fuck up like for the Map 😄

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

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.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: 29cae71

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

[trueadm]

We don't want to do this. Instead we need to do the same thing as what Map does for reading all keys.

[paoloricciuti]

We don't want to do this. Instead we need to do the same thing as what Map does for reading all keys.

Wait but aren't the sources needed for every element of the set anyway? If those were added one by one they would have their own source right?

[trueadm]

We don't want to do this. Instead we need to do the same thing as what Map does for reading all keys.

Wait but aren't the sources needed for every element of the set anyway? If those were added one by one they would have their own source right?

That doesn't mean they're reactive. They're only reactive when activated.

[paoloricciuti]

We don't want to do this. Instead we need to do the same thing as what Map does for reading all keys.

Wait but aren't the sources needed for every element of the set anyway? If those were added one by one they would have their own source right?

That doesn't mean they're reactive. They're only reactive when activated.

Yeah but isn't add doing the same thing here?

svelte/packages/svelte/src/reactivity/set.js

Line 98 in 9ea9f80

sources.set(value, source(true));

Are you saying the whole Set class should be reworked?

[paoloricciuti]

Oh or maybe you are saying to add a read_all here too that creates the signals if they are not there and is invoked in for each?

[trueadm]

Oh or maybe you are saying to add a read_all here too that creates the signals if they are not there and is invoked in for each?

Spot on

[trueadm]

We don't want to do this. Instead we need to do the same thing as what Map does for reading all keys.

Wait but aren't the sources needed for every element of the set anyway? If those were added one by one they would have their own source right?

That doesn't mean they're reactive. They're only reactive when activated.

Yeah but isn't add doing the same thing here?

svelte/packages/svelte/src/reactivity/set.js

Line 98 in 9ea9f80

sources.set(value, source(true));

Are you saying the whole Set class should be reworked?

Tbh we don’t need to create a signal here either really. We can just do it lazy in has and get_all

[paoloricciuti]

Oh or maybe you are saying to add a read_all here too that creates the signals if they are not there and is invoked in for each?

Spot on

Ok I will work on this tomorrow (but feel free to "hijack" the pr if you want)

[paoloricciuti]

@trueadm this should do it, tests are passing...my only doubt is if i'm over "reading all"...i've used the method in every read method, keys, values and entries. Given that keys is used by the iterator removing it there make literally tests fail and i assume you still want to read all for entries and values and all the read methods.

Let me know if i fumbled 😄

[Conduitry]

I've closed #11952 as a duplicate of the original issue - Do we feel like the test added here suffices to cover that case? I'm guessing so, because of its use of .size, but I wanted to check.

[paoloricciuti]

I've closed #11952 as a duplicate of the original issue - Do we feel like the test added here suffices to cover that case? I'm guessing so, because of its use of .size, but I wanted to check.

It should be...if you approve the deploy on vercel we can check 😄

[hanszoons]

Can confirm this solves #11952, thanks!

[Conduitry]

I had pulled down this branch and ran the REPL locally because I closed #11952, and reproduction in that issue seemed to be behaving correctly. My concern was specifically about regression tests, and whether we felt that the adjustments to the automated test sufficiently covered .size automatically updating.

[paoloricciuti]

I had pulled down this branch and ran the REPL locally because I closed #11952, and reproduction in that issue seemed to be behaving correctly. My concern was specifically about regression tests, and whether we felt that the adjustments to the automated test sufficiently covered .size automatically updating.

I'll add size too :)

[Rich-Harris]

Dammit, I was about to open an alternative PR. I was looking more closely at this and I don't think it's the right fix. Reverting