oliyh / re-graph

A graphql client for clojurescript and clojure
461 stars 40 forks source link

Support re-frame unwrap and in turn use event maps for mutations/subscriptions #81

Closed hipitihop closed 2 years ago

hipitihop commented 3 years ago

As of re-frame 1.1.2 there is a new interceptor available unwrap, this is the result of https://github.com/day8/re-frame/issues/644 which promotes 2-vector of [event-id payload-map] but because re-graph currently appends to the event vector, this cannot be used.

Proposal

re-graph handlers like :subscription could also use maps for parameters so that:

  (re-frame/dispatch [::re-graph/subscribe
                                    :my-subscription-id
                                    "{ things { id } }"
                                    {:some "variable"}
                                    [::on-thing]])

becomes:

  (re-frame/dispatch [::re-graph/subscribe
                                    {:id       :my-subscription-id
                                     :query    "{ things { id } }"
                                     :variables {:some "variable"}
                                     :event     [::on-thing {:other "my other payload"}]])

All results should be merged into the optional second arg of :event where the :on-thing handler can then use the unwrap interceptor. re-graph generally already uses maps for results i.e. {:data ...} or {:errors ...}.

To avoid key collisions in event maps, perhaps an additional optional argument {:event-path :results} could be supported, such that above example handler would get:

  {:other ...
   :results {:data ...}}

To make this an incremental change and to support backward compatibility, perhaps introduce a new parallel API e.g. ::re-graph/subscribe-m

Has this approach have some legs ? Any other ideas to improve on this ?

oliyh commented 3 years ago

Hello,

Thanks for bringing this to my attention. I knew of the plans to move to maps but I'm surprised this was introduced in a patch release of re-frame. I'm inclined to take the opportunity to move re-graph to 0.2.0 and make this a breaking change, because the current varargs in the subscription/mutation vector has gotten out of hand and this would be a good way of fixing it!

It will take a bit of re-working and documentation etc so will be a little while before I get around to this probably.

hipitihop commented 3 years ago

If you decide to move the API to use maps, which IMHO is preferable, then I agree it's best re-graph introduces this as a breaking ver change too and keep things simple not needing any backward compatibility.

If on the other hand you prefer to do things incrementally, then I wonder should we consider adding metadata to the handler parameter so internally you can support the new way of merging results/errors/data to the second arg map. Just spit-balling

e.g.

...
^:merge-arg-2 [::on-thing {:other "my other payload"}]
...
oliyh commented 2 years ago

Finally implemented! 0.2.0 🥳