fix: allow $store to be used with changing values including nullish values

[ramonsnir]

This aims to fix #7946 and fix #7555 by having utils.js/subscribe call the callbacks even if the store is nullish.

Related PR: #7693

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

[ramonsnir]

Thanks @Conduitry for identifying as a duplicate. I searched in GitHub but must have entered a wrong keyword combination or simply missed it (there are many results for words like "store", "undefined", etc.). 🙂

[ramonsnir]

@tanhauhau Do you know why the tests suddenly failed? It seems like a random failure of an unrelated test, only on macOS. I don't have permissions to re-run.

[tanhauhau]

it's okay. recently the tests are a bit flaky.

[baseballyama]

Almost LGTM but I added 1 comment.

[baseballyama]

1st argument of callback is a value of store, so in this case, the value is undefined right?
value of null/undefined is undefined.

Suggested change

	callback(store);
	callback(undefined);

[ramonsnir]

But null !== undefined. This is returning the same value passed, and preserving runtime correctness as checked by TypeScript.

[baseballyama]

Regarding store.value,
if store is null, store.value = null.value. value is undefined.
if store is undefined, store.value = undefined.value. value is undefined.

First argument of callback is value.

https://svelte.dev/docs#component-format-script-4-prefix-stores-with-$-to-access-their-values-store-contract

[ramonsnir]

I don't see an implicit conversion of null into undefined in the linked section, but my reading skills this morning aside - it seems to match the runtime behavior that exists today (minus this bug) 🙂 REPL confirming null -> undefined

If this is the desired behavior, the types for TypeScript need to be fixed too. This code right now fails to compile:

    let nameStore = null;

    let name: undefined;
    $: name = $nameStore;

Type 'null' is not assignable to type 'undefined'.

That's why I wrote the PR the way I did.

[dummdidumm]

I'm observing a difference in behavior with this change. This code:

<script>
  import { writable, derived } from "svelte/store";

  let name = null;
  $: d = derived(name, (e) => {
    console.log(e);
  });
  setTimeout(() => (name = writable("hi")), 1000);
  setTimeout(() => (name = null), 2000);
</script>

{$name}
{$d}

will print the following in the current version:

undefined
"hi"
undefined

With this PR it prints only

"hi"

The reason this happens is that pending inside the derived store turns truthy because the derived store is called while not being started yet.
I'm not sure yet if this is desired behavior and counts as a bugfix or if this should be seen as a breaking change (although, who really has code like that? 😅 ). Edit: should count as breaking.

Question is, if passing a falsy value to derived

• a) should be seen as an error
• b) should result in the behavior this PR has
• c) should function as before (in which case this isn't a breaking change, but would need a different change)

I slightly lean towards b or c.

Either way, we should have a test that codifies this behavior.

breaking change

[baseballyama]

c) should function as before (in which case this isn't a breaking change, but would need a different change)

IMO is c.
Because somehow derived store needs to know that the parent store is gone.

[dummdidumm]

That's a very good point, we can't do b) . I think I vote for a) then. The types already says "should be defined".

[Conduitry]

@dummdidumm Part of your sample code above, though, was $: d = derived(name, (e) => { ... });. This is how the derived would find out that name changed - the store would get re-created when name (not its contained value) changes. Am I missing something?

That's not to say I'm convinced that b) is definitely a better option than a). I remember feeling very conflicted about this when you brought it up last month. But I don't think we should rule out b) as technically infeasible, as far as I can tell.

[dummdidumm]

Mhm true... we need to find out what happens if you derive from multiple stores and one of them gets undefined.
If that works as expected you have me going back to favoring Option b) again 😅

[dummdidumm]

Added some tests, which was good to see that derived will with this change never finish:

const a = writable(1);
const b = derived([a, null, undefined], ([n, un1, un2]) => {
	// I will never end up in here, due to the inner `pending` variable of `derived` never being false
	return n * 2;
});

The fix is to either fill up any falsy stores with readable() (i.e. a store that contains a single undefined value) or to throw an error.

Which now begs the question, if this derived callback should run with [2, undefined, undefined], what should happen in the following situation:

derived(undefined, () => {
	// call this or not call this?
});

• If we should call it then we can also have a fallback readable(), which reintroduces the previous behavior, basically option c) from above.
• If we should not call it then it feels a bit inconsistent with the first example where derived is called, which makes me think any falsy value should be a runtime error, which would be option a)

--> option b) to me feels inconsistent now.

From a typings perspective having a falsy value is invalid already (TS will yell at you). From a backwards-compatible and code-golf perspective (you need less code for the fallback than for the error message) I still lean towards c)

Thoughts?

[vercel]

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

A member of the Team first needs to authorize it.

[dummdidumm]

We decided to error for falsy derived inputs