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

Fix subscribe warnings #15

Closed

Conversation

M-sleeper
Copy link

@M-sleeper M-sleeper commented Jul 10, 2023

This PR should solve the subscription warnings of re-frame, see related issue: #13

The cause of the warnings was, that re-frame's subscription system is not fully prepared for use-cases not in the reactive context. In this specific case re-pressed called the subscriptions in the event handler of keyboard events. One possible solution to avoid these warnings to access the app-db directly in an event, not via subscriptions in an effect handler. So, I changed the implementation of the effect handler such that it calls an event, that can access the app-db and dispatch the required further events.
One possible issue can be that instead of dispatch-synch I use the :fx [[:dispatch [...]]] return value of the event. This means that instead of directly processing the keyboard event it will put the events in reframe's event queue. This should not cause issues in most cases.
I tested the main functionalities locally but eventually there can be bugs in the change. Any review/feedback is welcome.

@gadfly361
Copy link
Owner

@M-sleeper Thank you so much for taking the time to review an open issue and to submit a PR, I really appreciate it!

It has been a while since I have written clojurescript or re-frame, so I am no longer current with best practices. Thank you for calling out the change from dispatch-sync to dispatch. In my opinion, honoring the guarantee that keypresses will be processed in synchronous order is more important than silencing the console warning for using the re-frame subscription incorrectly.

I am comfortable with a few options on how to proceed:

  1. you are welcome to fork this repo and carry the torch of re-pressed with an implementation that isn't synchronous. I'd be happy to link that repo from this one, with a note of the difference.
  2. the PR can be updated to retain dispatch-sync. Btw, is it possible to reference app-db directly and deref it to get its values instead of using a subscription? I assume this may violate some contracts of re-frame, but it has been a while, so I don't recall, but I imagine doing that would silence the warning from re-frame: Subscribe was called outside of a reactive context #13

@M-sleeper
Copy link
Author

M-sleeper commented Jul 17, 2023

@gadfly361 Thanks for the review.

About 2.
I see no other 're-frame accepted' way of accessing the app-db inside the event listener function, than using re-frame events. One 'not-accepted' way could be to use re-frame.db/app-db directly, but AFAIK using re-frame.db/app-db like that is quite uncommon/not-recommended, although it probably does not cause any problems. What do you think?

@gadfly361
Copy link
Owner

@M-sleeper thanks for the quick reply! Gotcha, makes sense that using app-db directly isn't recommended.

With that information, then I think where I am at is:

  • keeping dispatch-sync is higher priority than silencing warnings
  • since using i) a subscription outside of a reactive context and ii) using re-frame's app-db directly are both not recommended by re-frame's guidance, then I think going from i) to ii) is a lateral change (i.e., going from one unrecommended implementation to another)
  • since i) and ii) are both not recommended, then I prefer to just leave the current implementation and have developers see the console warnings. This way they are at least aware that the implementation isn't following Re-frame's guidance.

So I think this would cross off option 2 above.

@M-sleeper M-sleeper closed this Aug 1, 2023
@M-sleeper M-sleeper deleted the fix/fix-subscribe-warnings branch August 1, 2023 12:20
@M-sleeper
Copy link
Author

@gadfly361
I created this repository for the asynch version of re-pressed: https://github.com/M-sleeper/re-pressed

@gadfly361
Copy link
Owner

@M-sleeper Thank you! I added a note to your fork in the README

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.

None yet

2 participants