Skip to content
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.

Sign up for GitHub

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

fix: invalidate derived stores deeply #10575

Closed
wants to merge 6 commits into from
Closed

fix: invalidate derived stores deeply #10575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
91f5b84
add test to exercise semi-diamond dependency problem
Feb 11, 2024
f52adbc
Allow deep invalidation for stores
Feb 15, 2024
c5db044
Correct type docs
Feb 20, 2024
4e666de
Add revalidation logic to cover final test case
Feb 20, 2024
1e62e94
alternative fix
Rich-Harris Feb 20, 2024
78dc776
changeset
Rich-Harris Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tender-pants-run.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: invalidate derived stores deeply
59 changes: 42 additions & 17 deletions packages/svelte/src/store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { noop, run } from '../internal/common.js';
import { subscribe_to_store } from './utils.js';

const SUBSCRIBERS_SYMBOL = Symbol('subscribers');

/**
* @type {Array<import('./private').SubscribeInvalidateTuple<any> | any>}
*/
Expand All @@ -16,8 +18,11 @@ const subscriber_queue = [];
* @returns {import('./public').Readable<T>}
*/
export function readable(value, start) {
const w = writable(value, start);
return {
subscribe: writable(value, start).subscribe
// @ts-expect-error we don't want this to be on the public types
[SUBSCRIBERS_SYMBOL]: w[SUBSCRIBERS_SYMBOL],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be made non-enumerable then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symbols are already non-enumerable, no?

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

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

Copy link
Contributor

@trueadm trueadm Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

subscribe: w.subscribe
};
}

Expand Down Expand Up @@ -55,21 +60,19 @@ export function writable(value, start = noop) {
* @returns {void}
*/
function set(new_value) {
if (safe_not_equal(value, new_value)) {
value = new_value;
if (stop) {
// store is ready
const run_queue = !subscriber_queue.length;
for (const subscriber of subscribers) {
subscriber[1]();
subscriber_queue.push(subscriber, value);
}
if (run_queue) {
for (let i = 0; i < subscriber_queue.length; i += 2) {
subscriber_queue[i][0](subscriber_queue[i + 1]);
}
subscriber_queue.length = 0;
value = new_value;
if (stop) {
// store is ready
const run_queue = !subscriber_queue.length;
for (const subscriber of subscribers) {
subscriber[1]();
subscriber_queue.push(subscriber, value);
}
if (run_queue) {
for (let i = 0; i < subscriber_queue.length; i += 2) {
subscriber_queue[i][0](subscriber_queue[i + 1]);
}
subscriber_queue.length = 0;
}
}
}
Expand Down Expand Up @@ -103,7 +106,23 @@ export function writable(value, start = noop) {
}
};
}
return { set, update, subscribe };

/** @param {T} new_value */
function set_if_changed(new_value) {
if (safe_not_equal(value, new_value)) {
set(new_value);
}
}

return {
// @ts-expect-error
[SUBSCRIBERS_SYMBOL]: subscribers,
set: set_if_changed,
update: (fn) => {
set_if_changed(fn(/** @type {T} */ (value)));
},
subscribe
};
}

/**
Expand Down Expand Up @@ -156,7 +175,7 @@ export function derived(stores, fn, initial_value) {
throw new Error('derived() expects stores as input, got a falsy value');
}
const auto = fn.length < 2;
return readable(initial_value, (set, update) => {
const r = readable(initial_value, (set, update) => {
let started = false;
/** @type {T[]} */
const values = [];
Expand Down Expand Up @@ -186,6 +205,11 @@ export function derived(stores, fn, initial_value) {
},
() => {
pending |= 1 << i;

// @ts-expect-error
for (const s of r[SUBSCRIBERS_SYMBOL]) {
s[1]();
}
}
)
);
Expand All @@ -200,6 +224,7 @@ export function derived(stores, fn, initial_value) {
started = false;
};
});
return r;
}

/**
Expand Down
30 changes: 30 additions & 0 deletions packages/svelte/tests/store/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,20 @@ describe('writable', () => {
unsubscribe();
assert.doesNotThrow(() => unsubscribe());
});

it('ignores no-op sets', () => {
const store = writable(0);

let count = 0;
const unsubscribe = store.subscribe(() => {
count += 1;
});

store.set(0);
assert.equal(count, 1);

unsubscribe();
});
});

describe('readable', () => {
Expand Down Expand Up @@ -551,6 +565,22 @@ describe('derived', () => {
});
});
});

it('only updates once dependents are resolved', () => {
const a = writable(1);
const b = derived(a, (a) => a * 2);
const c = derived([a, b], ([a, b]) => a + b);

const values: number[] = [];

const unsubscribe = c.subscribe((c) => {
values.push(c);
});

a.set(2);
a.set(3);
assert.deepEqual(values, [3, 6, 9]);
});
});

describe('get', () => {
Expand Down
Loading