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.

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

Provide update as second argument parameter to stores #6737

Closed
stephane-vanraes opened this issue Sep 16, 2021 · 7 comments · Fixed by #6750
Closed

Provide update as second argument parameter to stores #6737

stephane-vanraes opened this issue Sep 16, 2021 · 7 comments · Fixed by #6750
Labels
runtime Changes relating to runtime APIs

Comments

@stephane-vanraes
Copy link
Contributor

Describe the problem

Currently the constructor for a writable store takes as a second argument function that is fired when the store gets a first subscriber, the return of this is fired when the store goes from 1 to 0 subscriber.
This function gives you access to the set function of the store, but in some cases it might be beneficial to actually have access to the update instead.

This would allow to do something like for example this

const store1 = writable()
const store2 = (() => {
   const { subscribe } = writable([], update=> {
     return store1.subscribe(value => {
       update(current => [...current, value])
     })
   })
   return { subscribe }
})()

Describe the proposed solution

Not 100% sure

  • change the signature ? the update function can be used a set as well (breaking change though)
  • add a third argument ?

Alternatives considered

Use some kind of construction with a temporary variable

const store1 = writable()
const store2 = (() => {
   let arr = []
   const { subscribe } = writable(arr, set=> {
     return store1.subscribe(value => {
      arr = [...arr, value]
       set(arr)
     })
   })
   return { subscribe }
})()

Importance

nice to have

@dummdidumm
Copy link
Member

Another possible non-breaking-change-way: writable(initialVal, (set, update) => ..)

@dummdidumm dummdidumm added the runtime Changes relating to runtime APIs label Sep 16, 2021
@rmunn
Copy link
Contributor

rmunn commented Sep 16, 2021

The example use cases look like something that would be easier with derived stores, at least if derived stores also got the same (set, update) treatment that @dummdidumm suggested:

const store1 = writable()
const store2 = derived(store1, (value, (set, update)) => {
  update(current => [...current, value])
}, []);

@stephane-vanraes
Copy link
Contributor Author

Not really, a derived store just maps one thing in another, it doesn't hold it's own state.

@rmunn
Copy link
Contributor

rmunn commented Sep 16, 2021

Derived stores do hold their own state, and if you use the (value, set) form, you can choose whether or not to update that state. E.g.,

const even = derived(count, (value, set) => {
  if (value % 2 === 0) {
    set(value);
  }
});

This isn't a simple map, because it's dropping half the values (assuming count is linked to a count.update(x => x + 1) button) and only updating the derived store half the time.

Derived stores using set are actually quite powerful. Here's a debouncer in just ten eight lines of code (two blank lines don't really count), and the core functionality is just two lines:

import { derived } from 'svelte/store'

const noop = (x) => x;

export function debounce(input, timeoutMs, fn = noop) {
	return derived(input, (newValue, set) => {
		const timeout = setTimeout(() => set(fn(newValue)), timeoutMs);
		return () => clearTimeout(timeout);
	});
}

@WHenderson
Copy link

The (non-breaking) solution I came up with for my store library was to have the update function as a property on the set function.

Would be great if svelte adopted the same syntax, but I'm obviously biased 😊

See: https://whenderson.github.io/stores-mono/modules/_crikey_stores_svelte.html#derive

@stephane-vanraes
Copy link
Contributor Author

@WHenderson just a heads-up you should check Svelte v4, this issue has been fixed as a "breaking change" and your custom store is likely no longer compatible with the expectation users might have. Relevant PR: #6750

@WHenderson
Copy link

Thanks @stephane-vanraes , I appreciate the heads up :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime Changes relating to runtime APIs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants