feat: pass update function to store setup callbacks

[rmunn]

This non-breaking change mostly non-breaking change (which is a breaking change for custom stores, see below) allows more complex store logic to be implemented, such as a derived store that accumulates a history of its parent store's values. The setup callback in readable and writable stores can now be either in (set) => { } form or in (set, update) => { } form. And the value-deriving callback in derived can now take an optional third parameter update in addition to the optional second parameter set.

Most of the use cases that would benefit from this are already possible, but would be made easier with this PR. For example:

const store = writable(0);
const history = derived(store, (newValue, set, update) => {
  update(values => [...values, newValue]);
}, []);

Instead of:

const store = writable(0);
const history = (() => {
  let values = [];
  return derived(store, (newValue, set) => {
    values = [...values, newValue];
    set(values);
  }, values);
})();

Fixes #4880.
Fixes #6737.

Breaking changes for custom stores

This is a non-breaking change for anyone using Svelte stores, as you can continue to write (set) => { ... } as well as (set, update) => { ... } in your stores' start callbacks. However, this is a breaking change for any code that creates a custom store. Your custom stores will now need to pass an update parameter to the start callback, as well as a set parameter. Most of the time it will be as simple as this:

diff --git a/src/runtime/store/index.ts b/src/runtime/store/index.ts
index e947fa074..afd4ccabd 100644
--- a/src/runtime/store/index.ts
+++ b/src/runtime/store/index.ts
@@ -92,7 +92,7 @@ export function writable<T>(value?: T, start: StartStopNotifier<T> = noop): Writ
                const subscriber: SubscribeInvalidateTuple<T> = [run, invalidate];
                subscribers.add(subscriber);
                if (subscribers.size === 1) {
-                       stop = start(set) || noop;
+                       stop = start(set, update) || noop;
                }
                run(value);
 
@@ -163,7 +163,7 @@ export function derived<T>(stores: Stores, fn: Function, initial_value?: T): Rea
 
        const auto = fn.length < 2;
 
-       return readable(initial_value, (set) => {
+       return readable(initial_value, (set, update) => {
                let inited = false;
                const values = [];
 
@@ -175,7 +175,7 @@ export function derived<T>(stores: Stores, fn: Function, initial_value?: T): Rea
                                return;
                        }
                        cleanup();
-                       const result = fn(single ? values[0] : values, set);
+                       const result = fn(single ? values[0] : values, set, update);
                        if (auto) {
                                set(result as T);
                        } else {

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

• Run the tests with npm test and lint the project with npm run lint

[rmunn]

I've included API documentation, but haven't updated the tutorial or the examples. I figured this feature is a bit advanced for the tutorial, and none of the examples seemed like they'd benefit from showing off the new update parameter of store callbacks. I can add an example if there's interest.

[stephane-vanraes]

For the derived store a trivial (and no uncommon) example would be to keep a history.

const store = writable(0);
const history = derived(store, (newValue, set, update) => {
  update(values => [...values, newValue]);
}, []);

But this makes me wonder, in the code you posted (copied here above) what would the initial value of values be ? Since there is nothing defined shouldn't ...values throw an error ?

[rmunn]

The initial value in that example would be [], passed as the third argument to derived.

[dummdidumm]

This needs to be a non-optional parameter, else TS will error here if you try to use update without checking for its existance first.
Which makes me think: Is this a breaking change for people implementing their own store contract? If so, yeah it needs to be an optional update parameter. @Conduitry what's your stance on this?

If we leave it like this, this means we need to adjust the other signatures a bit to type them so that update is not optional as it is in fact provided for readable/writable/derived

[rmunn]

Strange, the two unit tests I added ran just fine, and I'm not seeing any TS errors in VS code on my use of update in the tests. I thought it would be a breaking change (at least from Typescript's perspective if not from Javascript's perspective) to make this non-optional as then everyone who has written readable(0, (set) => { }); would get Typescript complaining that they didn't take the second parameter. But then, I don't know all the subtleties of Typescript; maybe it's fine with omitting parameters to functions because Javascript allows that?

[rmunn]

BTW, at no point in the Svelte code does the update function from StartStopNotifier get called; it's only passed to callbacks. And the callbacks can either take the update parameter and use it, or not take it in which case it would obviously be an error for them to have a call to update(x => x+1). But I don't think making this optional will actually produce any Typescript errors, and I do think that making it non-optional will be a breaking change.

[dummdidumm]

For function parameters, the inverse is true: If the function expects a function-parameter with x arguments, you can pass in a function with zero to x parameters.
You don't see errors because the repository runs TS in non-strict mode (no strict null checks).
See this playground for more: https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABACwKYBt1wBTDALkWwCdUBHQgZymJjAHMAaROABygH4qa76BKRAF4AfIgBucGABMBAbwC+AKEVpMObAJGIFfANyIA9AcRg4iVsTgAjdKgC2KjFmwlyzNlE2jZixH5bsAHRQcAAycADuqMQAwgCGlKga+kaI0ZbEiFaoEHEgiQFQiBBwIOhSWaiI4FKowHSoUoryfEA

[rmunn]

I see. And https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABACwKYBt1wBTDALkWwCdUBHQgZymJjAHMAaROAByiprvoEpEBeAHyIAbnBgATPgG8AvgCh5aTDmx8hiOTwDciAPR7EYOIlbE4AI3SoAtkoxZsJcuuHT9huDZhRYDFuyIMJRGJmaW1jaIqD5oxPKIiYikZAB0UHAAMnAA7qjEAMIAhpSoarrysjzyQA shows that omitting update will be no problem, so it won't break existing code. I'll make the change.

[dummdidumm]

It's a breaking change for everyone who uses StartStopNotifier to implement their own store contract: https://www.typescriptlang.org/play?#code/C4TwDgpgBAysCGAnYcD2YByrgEsBmOEiUAvFABQDOEwAXFJcIjgHYDmANFAK5gAm8YBHqNm7AJSkAfFABuqHHwDcAKBV5uLAMa5ULKAFsQAeQDuLNIgjkAFhAA291PXgsQXRtzx56cJCmB0LFwCIkkAbxUoaIZgLzxyAHJUez5E8SUoAHosqDArWRxUbkp7ECgCFgguFlRTKCJEVERKKAAjCC14EuheASEoHFba+vgoKwBHbhwrPjykeAMaIhUAXyA

[rmunn]

Was a little busy last week, but I've made the change to the StartStopNotifier type now. Now update is a non-optional parameter in that type, which will be a breaking change for anyone using the StartStopNotifier type for their own custom stores. I'll update the PR description to mention that it's a non-breaking change for anyone using stores normally, but a breaking change for anyone creating custom stores and using the StartStopNotifier type to describe them.

[dummdidumm]

The docs need to be updated (? needs to removed from the function signatures), apart from that this looks good. Also great job on finding the potential repos where it would break for other people. The fact that there are several repos already using that type though makes me think it might be better to merge this in Svelte v4, not now. @Conduitry thoughts?

[rmunn]

There are two kinds of breaking changes:

1. Changes the compiler will tell you about, and won't let you compile your code until you fix them
2. Changes the compiler will silently allow through and cause errors at run-time (also known as bugs)

I'm much happier making changes of type 1 than of type 2. AFAICT, for anyone using Typescript, this would be a type-1 change, because the Typescript compiler would catch that their custom store is no longer fulfilling the StartStopNotifier type.

However, anyone who's not using Typescript and is importing from one of those custom stores would find a runtime error when the update parameter of their callback was undefined (because the custom store code was never updated to pass (set, update) instead of (set)). So there could be some errors of type 2, which are best avoided.

I was hopeful when I saw #6737 (comment) that it might be possible to implement this in a non-breaking way, but now that I've thought about it, I'm afraid this is a breaking change after all. Because although it's not technically Svelte's responsibility to update your custom store's API if Svelte's stores API gains another feature, in reality there's an unwritten contract that custom stores will behave like Svelte stores, including in how they call callbacks. So although the callback API isn't documented in the official store contract (which only requires a subscribe function that returns an unsubscriber), in practice people will be depending on more than the official contract.

I do really want to get this in, though, because Svelte 4 would be a long time to wait, and this would make many use cases quite a bit easier.

[rmunn]

@dummdidumm wrote:

The docs need to be updated (? needs to removed from the function signatures)

Done in commit ef6e407.

[stephane-vanraes]

So the big thing stopping this for now is that if

• A created a custom store following the current store contract, and
• B using this custom store

B would expect to be able to use the update callback because this is what the new store contract would look like ?

Note that this change does not break current usage of custom stores but only future usage of said stores, so your code will not stop working overnight.

[dummdidumm]

This is one of the breaking changes, yes - although that is arguably the one I would actually be fine with. The (to me) bigger breaking change is that people using the store interfaces will get errors in their checks/IDE when using them.
Honestly, both these are not really big issue and the fix is probably in very few places and takes a minute maximum to fix. So I could be persuaded to accept this, but this is something I won't decide alone, so I'll wait on opinions of other maintainers.

[pngwn]

This a clear and obvious breaking change, there is no ambiguity.

The question is, are we comfortable that this change offers enough value to be worth the upgrade pain for consumers.

Strictly this should be v4 but we have shipped low impact breaking changes in minors before. One for the maintainers to discuss, but I wouldn’t expect much progress until we find the time to discuss that as efforts are mostly focused on kit at the moment.

[rmunn]

If this breaking change is considered for inclusion, I think it's worth considering #6777 as well, because that would also be a breaking change. Of course, #6777 might be rejected as "not enough value to be worth the extra cost", but if there's going to be a breaking change to the store API, might as well push both of those changes through at the same time to only pay the breaking-change price once.

[rmunn]

I've now created #6786 to implement #6777. Note that I created #6786 as a draft PR and that it includes the changes in this PR, because I believe this PR should be merged (or rejected) before #6786 is considered. If #6786 were merged first and this PR were merged second, then the derived callback signature would end up being (values, set, changedArray, update), which is hideously ugly. If update is going to be passed into the callback, it should be passed before any "what changed" Boolean array.

If this PR is accepted, I'll immediately update #6786 to remove its draft status, and rebase it off of master again so that there won't be any merge conflicts. Hopefully #6786 can then be considered for the same version number that this PR makes it into, because I'd greatly prefer to have both these PRs accepted or rejected together so that the derived callback signature only has to change once.

[rmunn]

I found a live example of why this is needed. At 9:52 of this Youtube video, someone wrote the following code to update a Svelte readable store from a Supabase subscription:

import {readable, get} from 'svelte/store'
// ... Supabase setup and so on ...
const posts = readable(null, (set) => {
  supabase
    .from('posts')
    .select('*')
    .then(({error, data}) => set(data));
  const subscription = supabase
    .from('posts')
    .on('*', (payload) => {
      if (payload.eventType === 'INSERT') {
        set([...get(posts), payload.new]);
      }
    })
    .subscribe();
  return () => supabase.removeSubscription(subscription);
})

Note how he wrote set([...get(posts), payload.new]) because he didn't have update(posts => [...posts, payload.new]) available, and he apparently didn't know how to use an IIFE to close over an array he could update. I don't know how often beginners make this mistake, but having update passed to store setup callbacks will mean a lot fewer people making it.

(I don't mean to pick on the author of that Youtube video, BTW; I simply happened to be watching it just now, noticed the inefficient use of get, and realized that that's exactly the use case I wrote this PR for.)

[chanced]

This is doable as-is.

function createStore() {
    const {set, update, subscribe} = writable(null, (set)=>{
        fetch('https://jsonplaceholder.typicode.com/todos/1')
            .then(response => response.json())
            .then(json => update(current => json))
       // return () => supabase.removeSubscription(subscription);
    })
    return {subscribe}
}

[rmunn]

@chanced wrote:

This is doable as-is.

For writeable stores, yes. But readable stores don't expose their update method, so the only way to create a read-only eventHistory type of store is to use an IIFE or apply this PR to give the start callback an update parameter.

Basically, I should have used readable instead of writable in the example in my PR description, because the example you gave works fine for writables but not for readables.

Edit: Replied too fast. I just realized that what you did was create a readable store since you only expose its subscribe method. So yes, that technique does work. However, I do think that a lot of people will not realize it's possible, and that passing an update parameter to the start callback is a lot nicer of an API.

[kwangure]

I've played with this for a few days now, creating different kinds of custom stores. Most are not ready for public eyeballs, but you can see some of my experiments at kwangure/storables.

The current StartStopNotifer interface is brittle and doesn't give room for customization. I think a more general interface would better. We'd now like to add an update function, what about 2 years from now when we need a delete function? My suggestion is to pass an object rather than a function.

interface Writer<T> {
	set(this: void, value: T): void;

	update(this: void, updater: Updater<T>): void;
}

...

- type StartStopNotifier<T> = (set: Subscriber<T>) => Unsubscriber | void;

+ type StartStopNotifier<T> = (writer: Writer<T>) => Unsubscriber | void;

This kills two birds with one stone since custom stores can now also pass custom functionality by extending the Writer interface. This would be a Svelte 4 change requiring an RFC, but I'd like feedback here first.

My suggested change does not preclude merging this PR into Svelte 3, but I agree with @chanced that much of the cited use cases of the PR are already possible (albeit hacky and non-obvious).

[WHenderson]

This non-breaking change mostly non-breaking change (which is a breaking change for custom stores, see below) allows more complex store logic to be implemented, such as a derived store that accumulates a history of its parent store's values. The setup callback in readable and writable stores can now be either in (set) => { } form or in (set, update) => { } form. And the value-deriving callback in derived can now take an optional third parameter update in addition to the optional second parameter set.

Most of the use cases that would benefit from this are already possible, but would be made easier with this PR. For example:

const store = writable(0);
const history = derived(store, (newValue, set, update) => {
  update(values => [...values, newValue]);
}, []);

I think the solution @chanced offered is the best solution, but I would also like to offer an alternative syntax:

i.e.
If the "set" function has two properties "set" and "update", then users can chose either of the following options:

const history = derived(store, (newValue, set) => {
 ...
}, []);

or

const history = derived(store, (newValue, { set, update }) => {
 ...
}, []);

This allows for all the functionality and ergonomics in the suggested solution, but provides a clear path for any future extensibility and avoids any confusion about argument order. I see there are requests for other information to be provided to the derived callback (such as "what changed" arrays etc), so staying future proof seems advisable.

Full disclosure: I already wrote a set of libraries with stores using this contract (https://github.com/WHenderson/stores-mono).
They are generally designed to be a drop in replacement for sveltes store implementation.

[dummdidumm]

We discussed this in the maintainer's meeting and decided against merging this before Svelte v4, since it's a breaking change for store implementers.

[WHenderson]

We discussed this in the maintainer's meeting and decided against merging this before Svelte v4, since it's a breaking change for store implementers.

Hi @dummdidumm, just wondering if my suggested syntax was discussed?
I have libraries implementing this approach but I would like to stay as close to the svelte standard syntax as possible.
I believe such a syntax would be a non-breaking change, though getting typescript to play nice can be a pain.

[benmccann]

@rmunn we've started a version-4 branch and I've changed this PR to target it. It looks like it needs a rebase though. Would you be able to update it? Thanks!

[dummdidumm]

I think we first should decide if we want to change this at all

[vercel]

@dummdidumm is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

[karimfromjordan]

In case there aren't enough examples yet, another use case I have encountered where this would be handy is a websocket or SSE store that connects inside start and pushes incoming messages into an array:

function create_sse_store() {
  let event_source;

  return readable([], (set, update) => {
    event_source = new EventSource('/sse');

    event_source.addEventListener('message', (event) => {
      update((messages) => [...messages, JSON.parse(event.data)]);
    });

    return () => event_source.close();
  });
}

[rmunn]

@benmccann Sorry I didn't see the notification last month (Apr 12); that was a busy time for me and I wasn't checking my email often enough. Thanks @dummdidumm for tackling the rebase and getting this merged into Svelte 4!