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

Observer should allow for multiple state objects at once #258

Open
funwolf7 opened this issue Jun 29, 2023 · 3 comments
Open

Observer should allow for multiple state objects at once #258

funwolf7 opened this issue Jun 29, 2023 · 3 comments
Labels
enhancement New feature or request not ready - design wanted Good idea but needs design work

Comments

@funwolf7
Copy link
Contributor

Currently, it is difficult to have a piece of code run whenever one of multiple different state objects change.

Let's say you have two states, State1 and State2, and you want to run an function whenever either of them change.

The most basic way is to use two Observers and run the code whenever any of them change, however this causes some problems.

  • Suppose both states are Computeds that are based on State3. Changing State3 would cause both to change one after the other, making the calculation run twice, the first time with only one of State1 and State2 changed. This can potentially cause issues where the first call doesn't function properly, as it may expect State1 and State2 to be similar in some way. It also leads to ambiguity, as the developer doesn't know which one will change first (and it may even be pseudo-random depending on the way the graph works), and could be problematic if the function is something like an HTTP call where you want to reduce the amount of times it runs.
  • The issues above will be made worse if Transactions / batching #150 gets added, as State1 and State2 may be set in the same bulk set/transaction, causing the same problems.
  • This is less important, but it is tedious to have to define a function to use it a couple times right below and then never again. You also have to call multiple functions to disconnect them all.

A current workaround is to create a Computed that calls use on all the state objects and returns something unique every time, then attaching an Observer to it. This gets rid of the problems as Computeds will only update once, after all their dependencies have updated.

Observer(Computed(function(use)
	use(State1)
	use(State2)
	return {}
end)):onChange(function()
	-- do whatever
end)

However this isn't an obvious solution, and has its own tedium of writing a use call for every state and returning a table. It is probably also less efficient than an official method would be.

Thus, I propose a way to have an observer take multiple state objects, and run the function only after all of them that will update actually update. I'm not sure what the best API implementation would be, but I'm sure there are plenty of elegant methods.

@funwolf7 funwolf7 added enhancement New feature or request not ready - evaluating Currently gauging feedback labels Jun 29, 2023
@krypt102
Copy link
Contributor

krypt102 commented Jul 3, 2023

Why not just

Observer({state1, state2, ...}):onChange(...)

From what the computed example has, it would still update every time either one of the two states changes? Unless I'm not understanding lol

@dphfox
Copy link
Owner

dphfox commented Jul 24, 2023

This is definitely something I've thought about. I would quite like to unify the design of Observers with that of Computeds etc, but the historical problem has been that the previous automatic dependency manager was incompatible with code that could yield (which Observers are explicitly designed to support).

This might not be such a problem now that we have the new use() callbacks. This may be worth thinking about.

@dphfox dphfox added not ready - design wanted Good idea but needs design work area: state and removed not ready - evaluating Currently gauging feedback labels Jul 24, 2023
@dphfox
Copy link
Owner

dphfox commented Aug 15, 2023

Related to #4 - the design of Eventual also provisions for use callbacks in yielding contexts. The behaviour of these two objects should be aligned with respect to how dependencies are captured after the first yield.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request not ready - design wanted Good idea but needs design work
Projects
None yet
Development

No branches or pull requests

3 participants