day8 / re-frame

A ClojureScript framework for building user interfaces, leveraging React
http://day8.github.io/re-frame/
MIT License
5.4k stars 715 forks source link

Warn user when calling subscribe outside of a reactive context #752

Closed dannyfreeman closed 2 years ago

dannyfreeman commented 2 years ago

I saw the help wanted tag on issue #740 and decided to open a PR for it

This will log warnings to the console every time a call to subscribe is made in a place it shouldn't be called, like event handlers.

Here is what the warning looks like in firefox: image

Right now it is printed as a warning. It may be more useful to print it as an error so the user can see the stack trace and find what code triggered the message.

dannyfreeman commented 2 years ago

I put this in the issue, but wanted to drop it here too. It may be worth figuring out how to change re-frame such that this warning is not necessary.

I made this branch a while back to explore what seems to be a reasonable solution to the problem: https://github.com/day8/re-frame/compare/master...dannyfreeman:bypass-subscription-cache-outside-reactive-context

In short, when the current call to subscribe is not happening in a reactive context and a subscription is not already cached, then bypass the subscription cache. Just return a new reaction. If the subscription was already cached then any code can grab and derefence that cached subscription (which is now only cached in reactive contexts, so something will eventually clean it up).

superstructor commented 2 years ago

Thanks @dannyfreeman 👍🏻 Code is good quality.

Please raise a new issue for the solution described in https://github.com/day8/re-frame/pull/752#issuecomment-1032139326 as my understanding is it would be best separated as a distinct future change to this even if it'll replace this warning.

dannyfreeman commented 2 years ago

Thank you @superstructor I followed up with a new ticket https://github.com/day8/re-frame/issues/753 and a draft PR to go along with it https://github.com/day8/re-frame/pull/754