fix: Avoid derived() store updates on invalid upstream state.

[mnrx]

Also: feat: Improve svelte/store's TypeScript type definitions.
Also: docs: Better document svelte/store and it's TypeScript type definitions.

---

This PR introduces a re-implementation of svelte/store. This implementation treats derived stores, in their capacity as a store subscriber themselves, with extra attention. This enables more accurate tracking of derived store validity and ensures that re-evaluation doesn't happen on invalid upstream state.

Currently, a change to a store's value only causes stores which are immediately dependent on it to be invalidated. Svelte does not propagate invalidation downstream to subsequent derived stores, and this can cause issues when two criteria are met:

• the store dependency graph forks and merges again at least once; and
• the two or more paths between the fork and merge points are of unequal length.

Svelte's current implementation correctly handles dependency diamonds, but such cases do not meet the second criterion; unequal path length.

Example

Consider the following dependency graph—A is a readable store, while B and C are derived stores.

stateDiagram-v2
  direction RL

  # classDef Readable font-weight:bold,stroke-width:3
  # class C Readable
  
  B --> A
  C --> A
  C --> B

Loading

Svelte's Current Implementation

In Svelte's current implementation, when the value of store A changes, stores B and C are each invalidated once. This is recorded in the pending variable in derived(). Immediately afterwards, both stores B and C re-evaluate, store C using the old value of store B that it recorded in its private values array during initialisation of the store graph.

Once stores B and C have changed value, they invalidate their subscribers. Store C has no subscribers to invalidate, but is invalidated itself by its dependency, store B. This causes store C to re-evaluate a second time, this time reaching the correct value.

The first re-evaluation of store C is wasted. This is arguably out of keeping with Svelte's efficiency, but is also—and more importantly—performed on invalid state. An exception could be thrown if, for instance:

• Store A held an array of objects.
• Store B held an index within that array.
• Store C held a property of the element of the array at that index.
• The array stored in store A were to shorten below the index in store B.

In this case, the index (store B) will become out of range, the object will become undefined, and property access on that value will yield a TypeError.

I suspect that JavaScript's loose typing allows a lot of these premature re-evaluations to go unnoticed in many real-world applications, generating short-lived NaNs and undefineds rather than throwing errors. I did encounter this issue organically, though.

This Implementation

This implementation starts the process of propagating a change in a similar manner, but completes all invalidation before re-evaluating any derived stores. When the value of store A changes, stores B and C are both invalidated. They each maintain an invalidation counter which tracks how many of their direct dependencies are about to update.

Since stores B and C are each being invalidated for the first time in this update cycle, they each invalidate their own derived-store subscribers. This means that store B invalidates store C a second time. At this point, the end of the graph is reached.

Store A now begins notifying subscribers properly of the new value. This is when regular subscribers, such as Svelte components, are notified. Store B notices that its only dependency has updated, and thus re-evaluates. Meanwhile, importantly, store C only decrements its counter.

Store B updating causes it in turn to notify its subscribers, including store C. This indicates to store C that the last of its dependencies that were expected to update has updated, so it re-evaluates.

Store C is only re-evaluated after all of its direct dependencies that need to be re-evaluated have been.

Implementation Details

Symbols are used to hide private methods and values on store objects. This approach could be replaced with underscore-prefixed property names, but such properties would be enumerable and may make users more likely to use these private properties despite them not being part of a public API. Object.getOwnPropertySymbols() can retrieve the symbols, however, so the properties are not truly private.

Alternatively, it may be possible to use Map or WeakMap to store a "shadow" object for each store which holds private data. Using symbols separates the public set() and update() functions from their implementations, however, potentially simplifying the proxy-like behaviour in higher-order stores such as spring() and tweened().

Status

This implementation passes all but one of the tests in the Svelte test suite, including passing a handful of new unit tests that the current implementation does not. The lint and build steps pass, too.

One test, "passes optional set and update functions", must be changed in order to pass. The test relates to this implementation's handling of multiple synchronous calls to set() and/or update() within a derived store's derive_value() function. This seems unlikely to affect any real-world applications, but—strictly speaking—it is a breaking change. A note has been added to the store documentation regarding this.

Breaking Change: Synchronous Calls to set() and update()

• Who: Some developers who pass multi-parameter derive_value() functions (typed ComplexDeriveValue<S, T> in this implementation) to derived().
• Migration: If the current behaviour is actually desired, make calls other than the first asynchronous, e.g., with setTimeout(..., 0).
• Why: derive_value() is executed during re-evaluation of stores, and a store setting its value two or more times would cause dependent stores to be invalidated more
• Severity: Low; I doubt this will cause many noticeable changes in real applications—if any, likely only in logging.

The type signatures of all exports of svelte/store have been changed. readable(), writable(), readonly(), and get() are now more strict about the types of the arguments passed to them. derived() is generally more flexible than before, but may also be more strict in some situations.

Breaking Change: Type Definitions

• Who: Developers who use svelte/store, TypeScript, and external stores together.
• Migration: Follow TypeScript's suggestions and the new documentation.
• Why: Improved type checking and typing of external stores.
• Severity: Low–medium; low incidence and easy to fix.
• Alternative: Retain more relaxed type definitions with worse type inference.

Breaking Change: ExternalReadable<T> Type

• Who: Developers who use Readable<T> in explicit type assertions.
• Migration: Change "Readable" to "ExternalReadable" in applications.
• Why: Allows separation of first-party Svelte stores and objects which only implement the basic contract.
• Severity: Medium; some incidence and easy to fix.
• Alternative: Prefix Svelte store types with Internal or similar and retain current naming.

The API of get() has also been extended in a non-breaking manner, and TypeScript definitions of all exports have been expanded and tested thoroughly (see tests/test.ts in mnrx/svelte-store, my TypeScript version of this module). Support for adding type information when using non-Svelte stores (e.g., RxJS Observables) is improved. New TypeScript-specific documentation has been added to better describe this interoperability.

New documentation in this PR could be moved to the tutorial section if that would be more appropriate. This PR could also be made an RFC if the maintainers think it ought to be one.

Changes

• Runtime
  • Add a new implementation of svelte/store, primarily changing the behaviour of derived().
  • Move get_store_value() (get()) to src/runtime/store/index.js, while still exporting the function from src/runtime/internal/utils.js.
  • Add allow_stale option to get().
  • Better document behaviour of safe_not_equal().
  • Fix small bug in tweened() to pass re-added unit tests.
• Tests
  • Add eight new unit tests covering derived().
  • Convert variable names to snake case and re-wrap comments in tests/store/test.ts.
  • Remove or better document 21 @ts-expect-error directives in tests/store/test.ts.
• TypeScript
  • Add new ExternalReadable<T> type to cast external stores to.
  • Add two new derived() overloads that make it easier to add type information to external stores.
• Documentation
  • Document TypeScript integration with derived().
  • Document on_start() in more detail.
  • Document new allow_stale option for get().
  • Use ts type for all snippets in order to show type annotations on hover.

Questions

• For the purpose of identifier casing, are function parameter names considered part of the public API? For instance, the allow_stale identifier I have added to get() appears in documentation and will appear in auto-completion pop-ups in users' editors, but will not appear in their source code. If yes, then allow_stale, on_start(), on_stop(), initial_value, and possibly other identifiers can be renamed.

• Is there a reason the unit tests for svelte/motion were removed in the Svelte 5 migration? Should they not be re-added?

• Does this impact store_get(), connect_store_to_signal(), and related functions? At least the first may be possible to simplify with allow_stale.

• Should EMPTY_FUNC not be used over inline noop()? Do these have different semantics?

Related Issues

I've searched open PRs and issues for anything related to svelte/store. These may be affected by this PR:

• Reorder the type parameters of derived #9084
  Partly resolved with explicit type cast of initial_value, e.g., null as unknown as number.
• Svelte supports rxjs observables but it is not reflected in types #8726
  Partly resolved with new derived() overloads with ExternalReadable<T>.
• Improve documentation on Stores internals #8298
  Partly resolved by changes to documentation and type definition comments.
• Tutorial - Expand Part 1 / Stores / Derived stores  #8125
  Partly resolved by changes to documentation.
• Provide the default store implementations as classes #7193
  Partly resolved by new allow_stale option for get().
• Expose additional implementation details for stores #6667
  Possibly resolved, or would be by exporting the symbols in this implementation.
  • Exporting the symbols from src/runtime/store/index.js would allow easier implementation of higher-order stores such as spring() and tweened().

Additionally, fixes or additions could be added for the following:

• toObservable or fromSvelteStore function to bridge  #8173
  A public toObservable() function, since the inverse is already implemented in wrap_store().
• Provide current value of store in second argument of stores #7520
  New current_value and final_value arguments to on_start() and on_stop() respectively (see Provide "what changed" array (of Booleans) to derived stores #6777 comment).
• fix: export types needed for defining custom stores #7358
  Could easily be merged into this PR.
• Provide "what changed" array (of Booleans) to derived stores #6777
  Fourth parameter for ComplexDeriveValue<S, T>, or replacement of second parameter with an object (like Provide current value of store in second argument of stores #7520).

Licence

This JavaScript version of the store is licensed under the MIT License. The TypeScript version in my other repository is licensed under the Open Software License version 3.0. Please tell me if this poses an issue.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 2dec711

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

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

A member of the Team first needs to authorize it.

[Rich-Harris]

Thank you, but this really should have been multiple PRs (or better still, an issue detailing the problem). I extracted the tweened store fix out into #10355, but there's still a whole bunch of stuff going on here, and it's hard to tell which bits can be reviewed independently.

The harsh reality is that when a maintainer sees a PR like this, they sigh, and put it to the back of the queue. Which means that merge conflicts arise, making it even less appealing to review. And then when it is eventually brought up to date (I just merged main to try and work on it), there's a high chance that tests will fail (which is exactly what happened here). The end result is that PRs like this one just don't tend to get merged.

I'm telling you this not to discourage you from contributing, but to help prevent future contributions from suffering the same fate.

[Rich-Harris]

I'm going to close this, as I don't see a path to merging in its current state, but please do feel welcome to create new issues and PRs for the individual features. Please avoid making drive-by formatting changes etc if you do.

[mnrx]

Thank you for looking at this, Rich.

I figured that this would be a challenge to merge, and so tried to document and justify the changes as best as possible. Initially, I did attempt to alter only the update logic in the existing implementation, but the mode of execution that it uses poses a problem. Critical information is not known at the right point in order to correctly decide whether or not to re-evaluate a derived store as one of its dependencies updates. Eventually, I decided this feature would be much easier to write and peer-review as a new implementation.

I don't think I can create PRs for "individual features"—this implementation was built to fix the premature re-evaluation issue, and the TypeScript improvements were incidental. I will bring this PR up-to-date with main if you think there's a prospect of it being merged, but I don't know how to split it into more reviewable parts.

Can you be more specific regarding "drive-by formatting changes"? I tried not to change anything without good reason.

I think Svelte really deserves to have a sound implementation for this. I lost a few days of work to my assumption that "of course Svelte doesn't re-evaluate derived stores before their depedencies". I was surprised that previous instances of this issue were (softly) dismissed as "niche". Others have implemented third-party stores with similar goals, such as warg, but I am keen to do what it take to get this upstream.

I have just filed an issue (#10376) using the "Example" section above and a reproduction. Please say if there's something else I should do.

[WHenderson]

Hi @mnrx, I too faced issues with some of the limitations of the default stores and implemented a separate library to solve them.
I agree it would be great to improve upon the default stores to resolve these but understand Rich's reservations.

I haven't looked into your patch in detail, but does it solve each of the issues listed here?

If you are still interested in moving forward with improving stores, I would be happy to help out. As you can see by my library (stores-mono) I have some experience with stores :-D

[mnrx]

I saw your libraries while looking for related issues—good work! Thank you for your interest.

Regarding your list, this implementation handles all five situations correctly, in my opinion. See the example component in my issue (#10376) or the three unit tests in this PR that directly relate to example e on your list. The "This Implementation" section in this PR should provide an overview of the algorithm this implementation uses to avoid re-evaluations on invalid upstream state.

My standalone TypeScript version works for my projects at the moment. I'm keen to get this or another implementation included with Svelte, but I need some assurance that the maintainers agree this is an issue that should be resolved before I spend time bringing this PR up to date.

[WHenderson]

To add to the conversation, I see the following limitations with the current stores contract/implementation:

1. The diamond dependency problem
2. Mixing svelte stores and 3rd party stores in the context of a diamond dependency
3. Greedy triggering of changes as defined by safe_not_equal in the svelte source code
4. Current typescript definitions don't fully propagate undefined
5. Exception handling

If we are to move forward, it would be worth seeing which issues we can get traction on (i.e. that the maintainers agree with and want resolved), and then to work out how best to divide them into discrete pull requests and in what order to avoid the issues you've faced in this PR.

To give more context to the list above:

# 2 Mixing svelte stores and 3rd party stores in the context of a diamond dependency
Details on this one are here: #6667
@Conduitry already weighed in on this one, but it may be worth re-raising it if I can better explain why the issue is something that should be solved.

# 3 Greedy triggering of changes as defined by safe_not_equal in the svelte source code
Svelte components can be defined with <svelte:options immutable={true} />, but stores have no parallel. This leads to an asymmetry and performance overhead when using immutable complex structures within a store.
Svelte users of libraries such as immer and mutative may well be facing this problem. I myself often store the majority of application state in a single complex structure and use writable derived stores (See @crikey/stores-select)

# 4 Current typescript definitions don't fully propagate undefined
A typed store can be left uninitialised, leading to subscribers receiving undefined when they expected otherwise.
e.g.

writable<number>().subscribe((x: number) => console.log('I was promised a number!', x));

Fixing this is simple (I can see two clear solutions), but whether this should be fixed or not is a question better left to the current maintainers.
It would be good to hear the thoughts on this from the maintainers whatever the outcome.

# 5 Exception handling
Subscriptions are handled through a single fifo-queue. An exception being thrown from a subscription breaks that queue and leaves the system in a fairly broken state.
Ideally subscriptions should be written to avoid throwing exceptions, but in the event they do, it may be favorable to allow some level of global exception handling to avoid breaking the rest of the application.

	// run when setting the value of a store
	// e.g. store.set(new_value);
	
	if (run_queue) {
		for (let i = 0; i < subscriber_queue.length; i += 2) {
			subscriber_queue[i][0](subscriber_queue[i + 1]); // <-- If this throws an exception, the rest dont get run at this time
		}
		subscriber_queue.length = 0; // <-- this will be skipped
	}
	// <-- the exception propagates to a caller who is unleiky to be equipped to handle it

[mnrx]

Regarding a route forward, I concur. In response to your points:

1. Diamond dependencies

   This is the purpose of this PR and Derived stores are re-evaluated on invalid upstream state #10376.

2. Mixing stores

   My implementation does not use the subscriber queue that the existing one does.

   When passed a third-party store as a dependency for a derived store, my implementation wraps it in a first-party store. A Map is maintained to avoid wrapping the same third-party store multiple times—in retrospect, this may better be a WeakMap. This covers third-party stores that act as a readable store, but may break if those third-party stores also depended upon other first-party stores. In this case, "break" means risk some re-evaluations on invalid upstream state.

   When it comes to authoring higher-order stores, my implementation could choose to expose its create_store function. The symbols used to access store methods could also be exposed publicly. I've not looked at this in enough detail to know if either of these things would be helpful. They would only be useful, however, when implementing higher-order stores that depend on other stores, i.e., derived-like stores, since the store wrapping technique mentioned above would handle it otherwise.

3. Greedy triggering

   I was slightly surprised by the logic of safe_not_equal, but I suppose it is a sensible default. I like the idea of an opt-in immutable mode that causes a given store to use a simpler inequality function. The not-a-number case is worth checking for, so this logic could work:

   function immutable_not_equal(old, new) {
     return !(old === new || (isNaN(old) && isNaN(new)));
   }

   I like the idea of stores-select, but does it strictly relate to this issue? I've tended to create a large object with many stores for property values when creating a central configuration store, so stores-select seems somewhat niche to me.

4. TypeScript definitions

   I just tested the TypeScript version of my implementation and found that writable<number>() raises a TypeScript error:

   Expected 1-2 arguments, but got 0. ts(2554)
   main.ts(227, 3): An argument for 'initialValue' was not provided.
   

   This can be circumvented by explicitly passing undefined as the initial value, i.e., writable<number>(undefined). When switching to the JavaScript implementation in this PR, the first example no longer raises an error. Here are the type definitions for writable:

   // `mnrx/svelte-store`, TypeScript
   function writable<T>(initialValue: T | undefined, onStart?: OnStart<T> | undefined): Writable<T>;
   // `mnrx/svelte/packages/svelte/store`, JavaScript
   function writable<T>(initial_value?: T | undefined, on_start?: OnStart<T> | undefined): Writable<T>;

   The key difference is the optional marker (?) on the initial_value parameter, which is generated automatically from the JSDoc comments in the JavaScript source. I used @param {T} [initial_value] ..., the square brackets meaning optional, as this is what svelte/store currently does (@param {T} [value] ...).

   I would support allowing end users to explicitly pass undefined as an initial value, even to a typed store, but passing nothing should perhaps not be permitted. The type definitions I've included offer better inference than the existing definitions, but I didn't want them to raise new errors where the existing definitions wouldn't have done.

5. Exception handling

   I didn't pay any specific attention to error handling. One exception breaking all store logic until the application is reloaded is not exactly pragmatic, so I support wrapping calls to derived store update functions and subscriber functions with some kind of error handler.

   It may be possible to queue any errors that occur while the store logic runs, then log the contents of that queue in one summary error once the store changes have propagated.

[Antonio-Bennett]

@mnrx @WHenderson Why don’t y'all move this to the issue so it doesn’t get lost and others can see the discussion

[WHenderson]

@mnrx @WHenderson Why don’t y'all move this to the issue so it doesn’t get lost and others can see the discussion

@mnrx , I agree with @Antonio-Bennett that these should be moved to issues to better track the discussion.
I've started this process and created the first issue here: #10451.

I'll create issues for the others and link them as I do.