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

Clarify subscriptions inside event handlers #64

Closed
egli opened this issue Mar 4, 2021 · 2 comments
Closed

Clarify subscriptions inside event handlers #64

egli opened this issue Mar 4, 2021 · 2 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@egli
Copy link
Member

egli commented Mar 4, 2021

According to this re-frame FAQ there shouldn't be any subscriptions inside event handlers. The present code base contains numerous such subscriptions.

I can see that a subscription is not needed here as we are not reacting to an event. Instead they are more used as getters into the global db and I was under the impression that this would be better than reaching into the global db with (get-in db [:foo :bar :baz]).

An alternative would be to define functions that abstract the path into the db away but from reading day8/re-frame#255 I get the impression that this is also not kosher:

to make a subscription value available in an event handler, it would have to be injected into the coeffects of that handler via an interceptor. We absolutely can't have an event handler reaching out and grabbing global values

They recommend to use coeffects but that seems pretty complicated for such a simple problem,

@egli egli added enhancement New feature or request question Further information is requested labels Mar 4, 2021
@egli egli changed the title Clarify the question re subsrciptions inside event handlers Clarify subsrciptions inside event handlers Mar 4, 2021
@egli egli changed the title Clarify subsrciptions inside event handlers Clarify subscriptions inside event handlers Mar 4, 2021
@plexus
Copy link

plexus commented May 12, 2021

coeffects aren't as complicated as they sound, but on the other hand I don't think using subscriptions inside event handlers is necessarily a problem. re-frame is quite opinionated on these things because that's what re-frame is, a set of opinions.

What you can do is split the subscriptions into a function that takes a db, and the actual subscription, so you can call the function elsewhere, if that makes sense for these subscriptions.

I think theoretically there might be a possibilty of inconsistencies, where the values you get from these subscriptions are based on a different db instance than the one your event handler sees, but in practice given that JavaScript is single threaded I don't actually think it's a problem.

@egli
Copy link
Member Author

egli commented Nov 15, 2021

Coeffects are indeed not that complicated, but according to the docs they are for things other than the app state (such as localstorage). Another down side of coeffects is that we'd define the path to the data in the app state twice still: first in the subscription and second in the coeffect which grabs the data out of the app db and adds it to the coeffects.

The other idea of splitting the subscription into an accessor function and the actual subscription seems more simple and keeps the path info really in one place.

@egli egli closed this as completed in 3ec0ad9 Nov 15, 2021
egli added a commit that referenced this issue Jun 16, 2023
Fixes #64

Subscriptions were used as a sort of an accessor function in event
handlers. This doesn't make sense. So the affected subscriptions are
now spit into an accessor function and the actual subscription
itself (which uses the accessor). The event handlers now just use the
accessor function which grabs the right data out of the app state db.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants