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

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

Closed
hipitihop opened this issue Mar 29, 2021 · 3 comments

Comments

@hipitihop
Copy link

hipitihop commented Mar 29, 2021

As of re-frame 1.1.2 there is a new interceptor available unwrap, this is the result of day8/re-frame#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
Copy link
Owner

oliyh commented Apr 2, 2021

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
Copy link
Author

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
Copy link
Owner

oliyh commented Jul 20, 2022

Finally implemented! 0.2.0 🥳

@oliyh oliyh closed this as completed Jul 20, 2022
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

No branches or pull requests

2 participants