Bug ? Subscription path must originally exist in server-side's resolver initial state about citrus HOT 30 CLOSED

Oh BTW, thanks a lot for having published a different version number. It won't break our stuff !

from citrus.

@DjebbZ Nice! I just updated docs to match 2.2.0 https://github.com/roman01la/scrum#server-side-rendering

from citrus.

I agree that if we would return to resolvers as a hash map we should drop vector as key. Let me think about it more, I'll get back soon :)

Also thank you for reporting this issue, this is something that no one should struggle with.

from citrus.

I agree, this is not very useful when having deep subscriptions. In fact I was thinking about possible solution, not hard enough yet 😅

In practice it's not recommended to use multiple deep subscriptions on the same entity, because it means multiple requests to database on the backend, for example. Instead it's always better to subscribe to top level entity, pull it out of your data source and let UI decide which parts of it should be actually used.

So in order to make deep subscriptions useful and performant, we need to optimize how data resolving is executed 🤔

Here's something to think about:

;; having these subscriptions
(scrum/subscription r [:user :name])
(scrum/subscription r [:user :friends])

;; define a hash map of corresponding resolvers that reflect state object
(def resolvers
  {:user {:name #(get-user-name)
              :friends #(get-user-friends)}})

But if we add top level subscription (scrum/subscription r [:user]) to previous two, this would not work very well.

I'm not sure yet how this can be solved. If we want to have per field resolvers it means there should be a way to batch subscription resolutions.

from citrus.

I think we should go with top-level only resolvers and pass the rest of the path into resolver function so it can decide which data should be returned.

(scrum/subscription r [:user :name])

(def resolvers
  {:user get-user})

(defn get-user [[field]] ;; <= [:name]
  (-> (db/get-user)
        (get field)))

Possible solution to avoid multiple requests to data storage could be the following:

• Always retrieve top-level entity
• When retrieved for the first time, cache it
• Pull entity from cache on subsequent resolution calls

I would implement top-level only resolvers thing in Scrum, but leave proposed optimization to users, to let everyone adapt it for their use case.

from citrus.

I think caching only leads to issues because of cache invalidation. The optimization part left to the users should in my opinion be the part totally left to the user. I doesn't have a place in Scrum, or it would get either too opinionated or too complicated (implementing caching is hard enough, and there are too many strategies to live in Scrum).

My view :

Given a resolver for path [:a] and a subscription for path [:a :b], the server should

1. Check if there's a subscription for path [:a :b]
2. Since there isn't, should check for each segment of path [:a :b] if there's a corresponding resolver.
3. It searches a resolver for path [:a], finds it, execute it (always, no cache) and retrieves data for [:a].
4. Since it knows there's no resolver for the rest of the path (see #1), it uses the rest of the original path [:a :b], here [:b] and does a (get-in state [:b]).

Why get-in ? This is what I expected from the API and docs, and what examples like [:users 0 :fname] suggested really hard. Since (at least for now) Scrum is tied to using an atom for the state (or at least an atom-like structure), and people generally use maps to store their state, using vectors as resolver keys and state retrieval make sense, they both share the semantics of get-in.

In the client-side, I'd love to be able to subscribe automatically to [:users 0 :fname].

Regarding unnecessary requests to data storage, the proposed solution leaves the option entirely to the Scrum dev. Since they have a reference to the reconciler, it's easy for anyone in the resolver function to check first in the state if there is some data (the atom is already the cache !) and decide what to do next. Nothing to implement in Scrum, all the flexibility to the dev.

Regarding using something else that get-in friendly vectors and other structures that atoms for the state, in this time of Datascript, specter and what-not, I don't know... I suggest you think about it for version 3 ?

from citrus.

What I'm doing right now is moving away from modeling resolvers as a hash map to multimethods. A hash map is basically a way to route subscription resolution based on provided path. IMO multimethods are a better fit for this. Also it seems more flexible to have resolver as a function.

Resolver type would look like this:

(deftype Resolver [state resolver path reducer]

  clojure.lang.IDeref
  (deref [_]
    (let [data (resolver path)] ;; <= call provided multimethod
      (when state
        (swap! state assoc-in path data))
      (if reducer
        (reducer data)
        data))))

And resolving multimethod:

(defmulti resolver (fn [[key]] key))

(defmethod resolver :user [[_ & path] req]
  (let [user (api/user (-> req :ui/route :route-params))]
    (match (vec path)
           [:id] (:id user)
           :else user)))

What do you think?

from citrus.

And client-side subscriptions ? Using get-in ?

from citrus.

On client-side it is different, Rum handles this automatically. I'm not entirely sure yet, need time to see how it goes.

from citrus.

2 questions regard your previous comment :

• how do you wire resolvers to the reconciler if it's not a hash map ?
• if resolver is a multimethod (which can run arbitrary code), what's the point of a having a reducer fn since the resolver method can already retrieves the exact piece of data ?

from citrus.

I just published 2.2.0-SNAPSHOT and updated example app.
Resolving multimethod: https://github.com/roman01la/scrum-ssr-example/blob/master/src/ssr/resolver.clj
Caching implementation: https://github.com/roman01la/scrum-ssr-example/blob/master/src/ssr/core.clj#L12-L19

how do you wire resolvers to the reconciler if it's not a hash map ?

See the above code snippets. Instead of a hash map you pass a function, that function accepts subscription path when called by Resolver. Because it is a multimethod you can route based on the first key in the path, for example.

if resolver is a multimethod (which can run arbitrary code), what's the point of a having a reducer fn since the resolver method can already retrieves the exact piece of data ?

I think reducers would be more common to use on client-side (in fact I doubt there's actual need in them). It is possible to do the same with resolvers being a hash map.

from citrus.

If I understood, you implement a simple caching strategy in user-space, not in Scrum. Nice !

Resolver : multi-method is much more verbose than hash-map, but indeed it provides max flexibility.

End of work day, I need to go. I'd be happy to check the code of scrum next week (small holidays :D). Keep up the good work ! I hope you'll find a good idea between client-side subscriptions and paths.

from citrus.

I'd like to try 2.2.0-SNAPSHOT with the new reconciler multi-method, but I have some troubles understanding the examples repo. Ring and Component obfuscate the core thing. I'd love a documentation update. Please 🙏 😉

from citrus.

Just so you know : even if I'm still using 2.1.0, I've managed to used the simple caching mechanism to prevent multiple requests server-side. As the reconciler is request-bound and our web framework make incoming requests thread-bound, the volatile cache works perfectly. Nice !

from citrus.

As I was blocked by the bug #13 I decided to upgrade my code from 2.1.0 to 2.2.0. The main difference being server-side resolvers as a hash-map in 2.1.0 and as a multimethod in 2.2.0.

Problem : multimethods are less flexible as hash maps in Scrum's case and prevents me from implementing my resolvers.

My use case : depending on the route being hit in the server, I create a different reconciler, with some resolvers being common to all routes and others being specific to a route. See the following example implementation:

(def common-resolvers
  {[:users] #(users-service/get)
   [:filters] (constantly filters-initial-state)})

(defn make-resolvers-route-1 [request]
  {[:products] #(products-service/get (-> request :params :ids))})

(defn make-resolvers-route-2 [_]
  {[:products] #(products-service/get-discounted-products)})

(defn make-reconciler-route-1 [request]
  (scrum/reconciler {:state (atom {})
                                :resolvers (merge common-resolvers (make-resolvers-route-1 request))}))

(defn make-reconciler-route-2 [request]
  (scrum/reconciler {:state (atom {})
                                :resolvers (merge common-resolvers (make-resolvers-route-1 request))}))

Imagine the two last functions being called sowehow in ring middlewares, pedestal interceptors and the likes.

Resolvers as data make it to decouple the subscription key and the implementation of how to retrieve the corresponding data. To rephrase, the decouple the domain from the route-specific implementation.

I spent one hour discussing how to the same with multimethods as required by 2.2.0, apart from some very ugly hacks (and not sure they would work) we didn't find a solution. For us, since resovlers are gonna be called with the subscription key (basically a keyword), nothing requires the rich dispatch capabilities of a multimethod.

How would you do it ? If you have no solution, I'd love a rollback to resolvers as hash-maps.

from citrus.

Is my understanding correct that from UI point of view products is the same thing and the server is calling different resolvers for example based on user role or something else that decides how to fetch data?

from citrus.

Exactly. In Scrum parlance (which is quite exact) products is the same domain, holding a list of products displayed by the same UI component. The server decided to fetch a different list of product depending on factors external to the UI (route and url params in our case, but could be anything else).

It would break the whole domain abstraction to tie the data fetching to a specific key in the resolvers map whereas they represent the same domain. In this regard, resolvers as data is perfect since data is always more compostable than functions. I would even say that since you said earlier that we should stick to top level subscription and do the data retrieval in the aggregate function, Scrum could use only a simple keyword in the resolver map, not a vector of keyword.

from citrus.

Ping ! Have you thought about it more ? Hash-maps or multimethods ?

from citrus.

I think hash maps is fine, as long as their purpose is justified.

What do you think about making keys as keys of top level subscriptions instead of vectors?

from citrus.

Vectors seem to imply that it's a path into a data-structure, whereas it's not. It's just the name of a domain. For naming things keywords are pretty good.

from citrus.

Great. I'll roll back to map impl. Or maybe you could do it? :)

from citrus.

Well I'd like to. But no time these days, just got back to work after holidays, now it's busydays :D

from citrus.

Same here :( I'll see if can find time to do it this week

from citrus.

@DjebbZ I just published 2.3.0-SNAPSHOT with hash map based resolvers and top level subscription keys, as we discussed it here above. Could you please give it a try?

Also added Request bound caching https://github.com/roman01la/scrum#request-bound-caching and Managing resolvers at runtime https://github.com/roman01la/scrum#managing-resolvers-at-runtime section to README :)

from citrus.

from citrus.

Hm, I'm not sure how to implement caching with hash maps now. Could you provide an example?

from citrus.

2 examples, based on the new stuff in the README:

(def anonymous-user
  {:top-products (memoize get-top-products)}) ;; simply memoize resolver

(def anonymous-user
  {:top-products (let [cache (volatile! nil)] ;; cache
                   (fn []
                     (if-let [data @cache] ;; cache hit
                       data ;; return data from cache
                       (let [data (get-top-products)] ;; cache miss, resolve subscription
                         (vswap! cache data) ;; cache data
                         data))))})

from citrus.

Ah, ok. It should be per resolver cache, now I get it. Thanks :)

from citrus.

@DjebbZ is this issue resolved already?

from citrus.

I've just realized that we're still using 2.1.0-SNAPSHOT since 2.2 was using multimethods. Haven't bumped yet, but I suppose it's your problem.
I could consider this issue closed, scrum is working quite good for us.

Will move to 3.0 Citrus soon hopefully.

from citrus.