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

Allow subscribe to be safely called from any context #754

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dannyfreeman
Copy link
Contributor

@dannyfreeman dannyfreeman commented Mar 6, 2022

The warn-when-not-reactive warnings are no longer valid if we bypass
the subscription cache when not in a reagent component, so they've been
removed.

This should address issue #753

This change probably requires some documentation cleanup

The `warn-when-not-reactive` warnings are no longer valid if we bypass
the subscription cache when not in a reagent component, so they've been
removed.

This should address issue day8#753

This change probably requires some documentation cleanup
@mbertheau
Copy link
Contributor

I love this. It seems to be an easy way – API-wise – to make values from the subscription calculation available in event and effect handlers.

What seems a bit problematic, or at least questionable (in the literal sense), is: what db state is used? In event handlers and possibly effect handlers you work with data passed into the function. subscribe uses a potentially different version of the app-db.

It seems to me that the API that one would want in that situation is (get-subscription-value db [:subscription parameters]), i.e. you pass in the db that should be used.

@dannyfreeman
Copy link
Contributor Author

There is only one db state in existence at a time, it will not be changed out from under running code in a single threaded environment. If you call a subscription in an event then the db value will be the same as the db being used in the event.

If we want something like get-subscription-value then a subscription can just be extracted into a function. Writing a generic utility for this would involve re-implementing subscriptions without de-referencing the global app-db. I've done something similar with re-frame to allow atoms other than app-db to be used. It was a lot of work. That is not something i want to waste my time on again because the re-frame maintainers don't seem interested in large patches like that.

@mbertheau
Copy link
Contributor

There is only one db state in existence at a time, it will not be changed out from under running code in a single threaded environment. If you call a subscription in an event then the db value will be the same as the db being used in the event.

Yeah. What I mean is that you might want to get the result of a calculation captured in a subscription based on the db that you're currently building a new version of in the event handler.

Some pseudo code demonstrating the mechanics I have in mind, not an actual use case:

(reg-sub :count-foo [db] (-> db :foos count))

(reg-event-handler :button-clicked [db new-foo]
  (let [db (-> db (update :foos conj new-foo)]
    (if (> 10 (get-sub-value db :count-foo))
      (assoc db :big? true)
      db)))

If we want something like get-subscription-value then a subscription can just be extracted into a function.

Well, not "just" because you have do extract all input subscriptions as well and compose everything together, and that might not be trivial, and/or impose significant boilerplate code when all subscriptions are written in a way that allows for that.

Writing a generic utility for this would involve re-implementing subscriptions without de-referencing the global app-db. I've done something similar with re-frame to allow atoms other than app-db to be used. It was a lot of work. That is not something i want to waste my time on again because the re-frame maintainers don't seem interested in large patches like that.

Sounds like a challenge! :)

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

Successfully merging this pull request may close these issues.

2 participants