-
Notifications
You must be signed in to change notification settings - Fork 67
Change subscriptionsFn
to take a model as an argument
#125
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really good first step, but I'm fairly sure this won't continuously re-jig the subscriptions every update with the new model, which is what it will need to do. Please correct me if I'm wrong.
helm.cabal
Outdated
@@ -1,5 +1,5 @@ | |||
name: helm | |||
version: 2.0.0 | |||
version: 3.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should stay at 2.0.0 as that hasn't actually been released yet
@z0w0 you are right, subscriptions should react on model changes. I'll try to dig in this direction |
@astynax should be able to change Let me know if you'll have the time to do that, otherwise I'll merge and adjust that myself :) |
But if you call |
Hmm, true. It would generate a new graph every time. Probably not ideal. |
I might be off here, but if the goal is to allow changing subscriptions at runtime, wouldn't be simpler to just allow the use of a Cmd to either delete / create a subscription, or to request the call to the subscription function (i.e. Cmd.updateSubscriptions) ? That way subscriptions would be re-calculated only when needed. |
Closes #116