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

[discussion] Expose update() and set() to get around forceUpdate() issue #1

Closed
jeffjose opened this issue Sep 8, 2020 · 5 comments
Closed

Comments

@jeffjose
Copy link

jeffjose commented Sep 8, 2020

Just chanced upon this package, and from the first look it reminded me a lot of svelte stores that I like a lot.

One thing that stood out on stream_state was forceUpdate() that's required when you mutate an object as opposed to replacing it. Svelte takes the opposite approach -- Dont do $count = newVal but do $count.update(newVal) and $count.set(newVal).

There are 2 paths here

  • Change stream_state to be like svelte that takes advantage of a popular API. I know this might be breaking change but if stream_state isnt used by a lot of people, then this might be a good opportunity.
  • Add update and set in addition. Now there'd be 3 ways to do one thing. I prefer having just 1 way for all cases, rather than the 2.

Thanks for reading!

@phimath
Copy link
Owner

phimath commented Sep 8, 2020

Yeah, forceUpdate() felt like bad api design when making it. I really wanted to figure out a way to detect mutation for any object type but was not able to do so. I agree that it would be better to have one way to do it and I don't believe many people are using this package yet besides me, so I'll implement this change.

One question though, what is the difference between set() and update()? Is there a reason to have both? If not I think it would be more clean to have only one way to always change state, perhaps 'set' because its shorter and still clear?

@jeffjose
Copy link
Author

jeffjose commented Sep 8, 2020

set is for overwriting the existing value, while update is for updating based on existing. In our case, I think set might be all what we need.


From the docs https://svelte.dev/docs#svelte_store

set is a method that takes one argument which is the value to be set. The store value gets set to the value of the argument if the store value is not already equal to it.

update is a method that takes one argument which is a callback. The callback takes the existing store value as its argument and returns the new value to be set to the store.

import { writable } from 'svelte/store';

const count = writable(0);

count.subscribe(value => {
	console.log(value);
}); // logs '0'

count.set(1); // logs '1'

count.update(n => n + 1); // logs '2'

@phimath
Copy link
Owner

phimath commented Sep 13, 2020

I'm implementing this now, but think that set isn't such a good name for the method, as VSCode sees it as a Dart keyword and colors it differently than other methods, which can be confusing to see.

Are you against using update as the singular method, which takes the value to be set?

@phimath
Copy link
Owner

phimath commented Sep 13, 2020

I added the update() method but as I'm updating the examples, I'm feeling it's maybe not the best solution and is straying away from the initial goals of the package ( a very simple / easy use state management for people struggling with reactive programming). I think accessing state with .state  and modifying it with .state =  feels  more simple, clean and consistent than accessing with .state and modifying with .update().

I agree that forceUpdate() feels bad and hacky (it is), but the workflow of using update() makes many (all?) tasks more difficult:

AppManager().counter++;
vs
AppManager().counter.update(AppManager().counter.state + 1);

Using update() gets even messier when modifying lists or custom types:

AppManager().wordList.state.add('test');
AppManager().wordList.forceUpdate()
vs
AppManager().wordList.update(...AppManager().wordList.state, 'test');
Even though the forceUpdate() way uses 2 lines, they are more clear and legible.

When you have StreamState objects of custom types, especially if you are nesting StreamState objects inside each other, this gets extremely complicated to manage with update().

I will leave in the update() method so that you and others who prefer that style can use it, but I think I will still recommend in the docs the original implementation.

I would very much love to figure out a way to detect mutation so that forceUpdate() is not required.  I will spend more time trying to come up with a way to do this.   I think that will be the cleanest and nicest approach.

@jeffjose
Copy link
Author

Sorry, I couldnt get to this in time. In svelte, update takes a callback, so it isnt as awkward as some of the examples you have.

That said, after using stream_state for sometime, I havent run into update() issue as much as I thought it would. I'm OK fi you want to close this one out.

Thanks again for taking a look!

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

No branches or pull requests

2 participants