Skip to content

Provide the default store implementations as classes #7193

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.

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

Closed
ponderingexistence opened this issue Jan 27, 2022 · 6 comments
Closed

Provide the default store implementations as classes #7193

ponderingexistence opened this issue Jan 27, 2022 · 6 comments
Labels
feature request runtime Changes relating to runtime APIs

Comments

@ponderingexistence
Copy link

ponderingexistence commented Jan 27, 2022

Describe the problem

Rationale:

As someone who has written quite a number of rather complex custom stores, two fundamental pain points have been the lack of polymorphism and lack of access to the store's current value in the custom store's implementation.

Because of this, I'm proposing that the default store implementation be provided as a class, precisely because this makes possible the two aforementioned scenarios.

Some real-world use cases wherein polymorphism and current value access would be essential to building certain custom stores are described below:

1. Access to the current value without subscription

This is a very common requirement. Imagine a timer function that returns a custom store that represents a countdown timer:

export function createTimer(shouldElapseMs: number, percisionMs = 50) {
    let state = {
        running: false,
        finished: false,
        remainingMs: shouldElapseMs,
        elapsedMs: 0,
    };

    const store = writable(state, () => stop);

    function start() {
        if (get(store).running)
            return;

        if (get(store).finished)
            reset();

        setInterval(() => {
            store.set({
                ...get(store),
                remainingMs: get(store).remainingMs - percisionMs,
                elapsedMs: shouldElapseMs - get(store).remainingMs,
                finished: get(store).remainingMs - percisionMs <= 0,
            });
            if (get(store).finished) stop();
        }, percisionMs);

        store.set({
            ...get(store),
            running: true,
        });
    }

    function stop() {
        // irrelevant...
    }

    function reset() {
        // irrelevant...
    }

    return {
        subscribe: store.subscribe,
        start,
        stop,
        reset,
    };
}

As you can see, we're using the get() function here (imported from svelte/store) each time we need the current value of the underlying store. However, the get() function is inefficient, as explained here in the docs:

This works by creating a subscription, reading the value, then unsubscribing. It's therefore not recommended in hot code paths.

So, in an example like the timer where we do have a "hot code path", (namely inside the setInterval callback, which is going to be executed every 50 milliseconds), this inefficiency is especially significant.

You might also think: "Well, we can't we just subscribe to the underlying store in our function?!"
Well, we can, but that would mean the underlying store will always have at least one subscriber, which in turn means that the function you return from the start callback, which only runs once when the last subscriber unsubscribes, will never be called. So this is certainly not a feasible solution either.

The writable function currently does store the current value as a local variable:

function set(new_value: T): void {
if (safe_not_equal(value, new_value)) {
value = new_value;

which of course means it doesn't expose it to the outside. If this, however, was a class, this variable could've been exposed as a protected property, which would give access to it to deriving classes, and that would beautifully solve this problem.

Note that various (ugly) workarounds like this are currently being used across the Svelte ecosystem, here's an example.

2. Polymorphism

Suppose you want to write a function that creates a writable store whose value is synchronized across all the open tabs (or browsing contexts, to be more accurate).

import { writable, type Writable, type StartStopNotifier } from 'svelte/store';

export function crossTabStore<T>(channelName: string, initValue: T, start?: StartStopNotifier<T>): Writable<T> {
    const underlyingStore = writable(initValue, start);

    const channel = new BroadcastChannel(channelName);
    channel.addEventListener('message', (e: MessageEvent<T>) => underlyingStore.set(e.data));

    return {
        ...underlyingStore,
        set(value) {
            underlyingStore.set(value);
            channel.postMessage(value);
        },
    };
}

This looks simple and straightforward. There is, however, a subtle bug in here: If the user of the returned store uses the update function to set a new value for the store, the value would NOT be synchronized. Why? Because our overridden custom set method, which in turn calls channel.postMessage(...) isn't called, even though the built-in update method does call set internally:

function update(fn: Updater<T>): void {
set(fn(value));
}

since there's no polymorphism when we're dealing with plain objects like this, it's actually the default set function that update is calling, and not the overridden/custom one.

This means we'd have to implement a very awkward custom update function too:

import { writable, type Writable, type StartStopNotifier } from 'svelte/store';

export function crossTabStore<T>(channelName: string, initValue: T, start?: StartStopNotifier<T>): Writable<T> {
    const underlyingStore = writable(initValue, start);

    const channel = new BroadcastChannel(channelName);
    channel.addEventListener('message', (e: MessageEvent<T>) => underlyingStore.set(e.data));

    return {
        subscribe: underlyingStore.subscribe,
        update(updater) {
            let newValue: T;
            underlyingStore.update(currentValue => {
                newValue = updater(currentValue);
                return newValue;
            });
            channel.postMessage(newValue);
        },
        set(value) {
            underlyingStore.set(value);
            channel.postMessage(value);
        },
    };
}

If, however, the default store implementation was a class that could be extended, we could've simply overridden the set function, and then whenever the update function calls set internally, polymorphism would kick in and everything would work as expected:

class CrossTabStore<T> extends Store<T> {
    private channel: BroadcastChannel;

    constructor(channelName: string, initialValue: T, start?: StartStopNotifier<T>) {
        super(initialValue, start);
        this.channel = new BroadcastChannel(channelName);
    }
    
    set(value) {
        super.set(value);
        this.channel.postMessage(value);
    }
}

Which one is cleaner? I reckon the difference is clear.

Describe the proposed solution

What I'd suggest we do is extract the default store implementation, which currently resides in the writable function here, into two classes (Store, and ReadonlyStore to differentiate them from the Writable and Readable interfaces, maybe? This is debatable though), and also keep the writable and readable functions, so as to avoid any breaking-change.

The writable function could still continue to be used whenever you want to simply create a store, and the new class, on the other hand, would be meant to be extended and used for creating custom stores.

And again, this change would not be breaking.

Regarding what would change in the tutorial, all I would change is the tutorial on custom stores, to start talking about these classes and how they are what should be used when creating custom stores.

Alternatives considered

As I mentioned, currently you have to utilize dirty workarounds to make up for these fundamental limitations, such as the ones I demonstrated above. So, this would indeed make creating custom stores much more efficient and developer-friendly.

Importance

would make my life easier

@Conduitry
Copy link
Member

I don't think we want to have two ways of doing the same thing. The default store implementations are just examples that could well live in userland. Nothing is really an integral part of the framework apart from the store contact. If someone wanted to publish their own store classes that they feel make it easier to create stores, they're of course free to do so. Add long as the resultant objects abide by the store contact, it wouldn't matter where they came from.

@ponderingexistence
Copy link
Author

ponderingexistence commented Jan 27, 2022

That's right, I'm definitely aware that you can create these classes yourself if you want, but that means you'd then have to mostly duplicate the work already done here. And that doesn't make much sense.

My suggestion here is that I think it would be very useful if Svelte provided its standard, canonical implementation of stores as classes that could then be extended by users, and also if it started promoting this as the recommended way to create custom stores, since as I explained, this is in fact the superior approach.

@bluwy
Copy link
Member

bluwy commented Jan 27, 2022

Seems like a duplicate of #6640

@ponderingexistence
Copy link
Author

ponderingexistence commented Jan 28, 2022

Seems like a duplicate of #6640

Well, kind of.
The OP there didn't really elaborate on the various reasons as to WHY classes would be superior, so the suggestion was dismissed more or less out of hand.
Here, on the other hand, I've outlined exactly why this has several key advantages over the current approach. If you believe any of the points I made are invalid, I'd love to hear your thoughts.

@bluwy
Copy link
Member

bluwy commented Jan 29, 2022

Access to the current value without subscription

I agree that get() makes things awkward, but that's because of how the store contract works, which is an object with a subscribe method. I think the better solution is to have writable specifically to have a .get() function, but that's a topic for another day.

Polymorphism

It does look like a class would make the implementation cleaner, though it relies on the fact that we know the Store update() calls set() internally. I'm not sure if this is a safe assumption, if we want to extend other external stores, they might not implement the logic the same way. In the end, we have to re-implement update() too with the same tedious code.


Though with all these said, I don't think classes are bad. There are trade-off between classes and functions, e.g. classes can't be spreaded. And in my experience, I don't always extend a store, my custom function may use multiple stores in which the syntax between classes and functions aren't much different.

For now, I think a better path is to implement these class alternative in userland first, and if it proves to be more handy than functions, then this proposal would have a solid start from the get-go.

@dummdidumm
Copy link
Member

Closing as won't do - as pointed out, Svelte stores are more of a reference implementation and people are free to create their own. Also, Svelte 5 makes stores less prominent because universal reactivity can be coded with runes.

@dummdidumm dummdidumm closed this as not planned Won't fix, can't repro, duplicate, stale Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request runtime Changes relating to runtime APIs
Projects
None yet
Development

No branches or pull requests

4 participants