fix: invalidate derived stores deeply

[Rich-Harris]

This is an alternative to #10557 which solves the problem in a slightly different way, by giving the derived store access to its own subscribers (so that in can invalidate them deeply). It's the same solution, at a high level, but with arguably more minimal changes to the visible API surface area, and a bit less code (a net increase of 25 LOC rather than 75)

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: 78dc776

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]

Should this be made non-enumerable then?

[Rich-Harris]

symbols are already non-enumerable, no?

obj = {
  [Symbol()]: 1,
  foo: 2
};

for (const key in obj) {
  console.log(key, obj[key]); // logs `foo 2`
}

[trueadm]

Object.assign() copies enumerable symbols as does object spreading.

[Rich-Harris]

I think it's probably harmless — people aren't really spreading or assigning the results of calling writable and readable, and there's not much damage that could be caused by people inadvertently getting a reference to the subscriber list. Feels like overkill to use Object.defineProperty here

[mnrx]

I tested this implementation against a few test cases I created for my own implementation. I think this change introduces a regression: If the derived store's function fn returns the same value it did previously, the store still notifies its subscribers.

The following test case generates [1, 2, 2, 2, 3] rather than [1, 2, 3].

it('does not notify subscribers if the derived value is unchanged', () => {
  const values: number[] = [];

  const number = writable(1.1);
  const round_number: Readable<number> =
    derived(number, ($number) => Math.round($number));

  const unsubscribe = round_number.subscribe((value) => values.push(value));

  assert.deepEqual(values, [1]);
  number.set(1.8);
  number.set(2.2);
  number.set(2.4);
  assert.deepEqual(values, [1, 2]);
  number.set(2.9);
  assert.deepEqual(values, [1, 2, 3]);

  unsubscribe();
});

Also, in case you aren't aware, this PR appears to revert the behavior introduced in PR #3219. I much prefer this behaviour, but it's worth noting. As I gather, the original motivation for that PR was performance, which has me considering to build a set of more complex store graphs for benchmarking.

[WHenderson]

Haha, you beat me by a few minutes @mnrx :-D

I'm not sure the best way to contribute to the existing PR, so for now here is a change to one of the test cases to catch the issue:

it('derived dependency does not update and shared ancestor updates', () => {
	const root = writable({ a: 0, b: 0 });

	const values_a: string[] = [];
	const values_b: string[] = [];

	const a = derived(root, ($root) => {
		return 'a' + $root.a;
	});

	const unsubscribe_a = a.subscribe((v) => {
		values_a.push(v as string);
	});

	const b = derived([a, root], ([$a, $root]) => {
		return 'b' + $root.b + $a;
	});

	const unsubscribe_b = b.subscribe((v) => {
		values_b.push(v as string);
	});

	assert.deepEqual(values_a, ['a0']);
	assert.deepEqual(values_b, ['b0a0']);

	root.set({ a: 0, b: 1 });
	assert.deepEqual(values_a, ['a0']);
	assert.deepEqual(values_b, ['b0a0', 'b1a0']);

	unsubscribe_a();
	unsubscribe_b();
});

The issue here is that the change to the internal set logic is necessary to tell all subscribers/decedents that the value is no longer invalid and can safely be used. This in turn means that regular subscribers (those unaware of the invalidation semantics) are getting duplicate signals.

Without a separate "revalidation" api, the only alternative I see is to detect subscribers that don't provide an invalidation handler and treat them differently, but this feels like a kludge with limited utility.
Overall, I think I would prefer more formal revalidation semantics.

Some other notes on the implementation:

1. Performance: The new invalidation handler in derived should probably check to see if the store is already invalid before propagating more invalid notifications.

something like:

() => {
	const signal = !pending;
	pending |= 1 << i;

	if (signal) {
		// @ts-expect-error
		for (const s of r[SUBSCRIBERS_SYMBOL]) {
			s[1]();
		}
	}
}

2. Hiding the subscriber symbol limits extensibility
   With the subscribers hidden behind an unexported symbol it wont be possible to create 3rd party derived stores with matching functionality.
   I would recommend at least exporting this symbol to aid 3rd party developers - even if it remains undocumented.

users found regression

[WHenderson]

I had a bit more of a think about this and realised there is an added advantage to an accessible "subscribers" list.
Notably, it could allow the creation of a transaction function akin to a database transaction.

e.g.

const a = writable(0);
const b = writable(0);
const c = writable(0);
const d = derived([a,b,c], ([a,b,c]) => a + b + c);

d.subscribe(d => console.log(d));

function transaction(stores, callback) {
  stores.forEach(store => invalidate(store));
  callback();
  stores.forEch(store => revalidate(store));
}

transaction([a,b,c], () => { 
  a.set(1); 
  b.set(2);
  c.update(c => c+ 1);
});

In this example, the derived store wont be updated until each of a/b/c are all updated instead of updating 3 times.

I haven't personally come up against the need for transactionally setting a group of store values, but I can see the utility.

[Rich-Harris]

On reflection: I'm going to close this and #10557, because ultimately the real solution to this problem is 'use signals'. While stores will continue to exist in Svelte 5, they're not something we want to invest in. This PR introduces a regression while #10557 involves API changes; both involve more code. I think it makes more sense to keep stores working as they do in Svelte 4, however imperfect that may be in these cases, and push people towards the modern way of doing things. Thank you @WHenderson for the time spent working on this and the thorough explanation.

[WHenderson]

On reflection: I'm going to close this and #10557, because ultimately the real solution to this problem is 'use signals'. While stores will continue to exist in Svelte 5, they're not something we want to invest in. This PR introduces a regression while #10557 involves API changes; both involve more code. I think it makes more sense to keep stores working as they do in Svelte 4, however imperfect that may be in these cases, and push people towards the modern way of doing things. Thank you @WHenderson for the time spent working on this and the thorough explanation.

No worries @Rich-Harris , I understand prioritising Svelte 5 and signal usage. I'm looking forward to using them :-)