Extract globals from re-frame state #3285
Conversation
There was a problem hiding this comment.
This looks pretty much ready. I think we should consider the spa, app & globals and possibly decide to have just two.
Also we are using direct atom references, but if it reads badly we could use helper functions instead. As long as everything is found with rems.globals then we can change in the future.
| @@ -21,7 +21,7 @@ | |||
|
|
|||
| (defn save-roles [response] | |||
| (when-let [roles (-get-response-header response "x-rems-roles")] | |||
| (rf/dispatch [:set-roles (map keyword (split-words roles))])) | |||
| (reset! rems.globals/roles (set (map keyword (split-words roles))))) | |||
There was a problem hiding this comment.
One difference with using a direct reference here is that there can't be dependency circles, where in re-frame there was one layer of indirection.
The direct reference from rems.ajax to the globals seems a bit odd anyway.
| ;; default language first | ||
| (sort (comp not= (:default-language db)) | ||
| (:languages db)))) | ||
| (defn- validate-lang [lang] |
There was a problem hiding this comment.
I think this could simply be
(defn- validate-lang [lang]
(contains? (set @rems.config/languages) lang))
src/cljs/rems/user_settings.cljs
Outdated
| (when (and (not (:user-settings db)) ; first time | ||
| (not (validate-lang (get-language-cookie)))) ; e.g. unsupported language in cookie | ||
| (save-user-language! (:language user-settings))) |
There was a problem hiding this comment.
Doesn't save-user-language! already do some of this checking?
Most initialization happens in app.cljs and spa.cljs. User settings and language contained a lot of re-frame event spaghetti, which is now reduced to 1 event and a few functions that can be reference jumped into. Language reaction (getter) is slightly simpler, at the expense of doing more during fetching user settings.
Part of the refactoring effort to move static state from re-frame to globals. As language is pretty much considered to mean "current user language" everywhere, passing it as props through multiple components does not make a lot of sense anymore. Container/view separation might still make sense for some UI parts.
Checklist for author
Remove items that aren't applicable, check items that are done.
Reviewability
Backwards compatibility
Documentation
Testing
Follow-up