compassus / compassus

A routing library for Om Next.
https://compassus.github.io/compassus/doc/1.0.0-alpha3/
Eclipse Public License 1.0
110 stars 18 forks source link

Optionally disable parser route dispatching #12

Closed molst closed 7 years ago

molst commented 7 years ago

The official remote sync tutorial is currently THE working tutorial for how to pretty easily get remote reads working with om.next. The other tutorials aim to go deeper, but are not finished.

The official tutorial requires that the read function dispatches on all keys in the query tree, but when using compassus only the top route keys get dispatched. This makes it much harder to get remote reads working because you either need to use an exotic dispatch function (looking at a larger query structure) or parse the whole ast for the route in the send function when putting remote queries to the send channel.

If compassus could be configured to dispatch on all subkeys in the query tree it would likely play well with the mainstream usecase in the official tutorial.

Peeja commented 7 years ago

I think you may be missing something significant about how the parser works. The parser, ordinarily, doesn't dispatch on all of the subkeys in the query. It only dispatches on the keys at the top level of the query. So, if your root query is

[:a
 {:b [:b/foo
      {:b/bar [:bar/qux]}]}
 :c]

your read will be called three times, with :a, :b, and :c. The :b call will have the query set to [:b/foo {:b/bar [:bar/qux]}]. If you can resolve that whole query all at once, say with a call to db->tree, you can do that. Otherwise, you may want to call the parser recursively from there to dispatch your read on :b/foo and :b/bar.

The only effect Compassus has is that it wraps your query to be under the route key. That is, if the query above is the query of your route component, the actual query your parser sees is

[{:some/route
  [:a
   {:b [:b/foo
        {:b/bar [:bar/qux]}]}
   :c]}]

This is why the parser only invokes your read once, and only for the key :some/route.

It makes sense to want your read to dispatch on the keys of the route query, and ignore the route key itself. I don't think there's a case where it makes sense for your read to get the route key and the keys nested under it. Your read would be delivering a value for, for instance, :a in two different places: once as part of the :some/route read and once as the :a read. You really want one or the other.

To do this at CircleCI, we've done the following:

(defn flattening-parser
  "Takes a parser. Returns a parser which ignores the root keys of the queries
  it's given, and reads the keys joined to those keys as if they were root keys,
  using the given parser.

  This is particularly useful when using Compassus. Compassus has its own parser
  which calls your parser. The root query it passes to your parser has a single
  key, which is the current route, which is joined to that route component's
  query. That is, if the current route is :app/home and that route matches the
  following component:

  (defui Home
    static IQuery
    (query [this]
      [{:some/data [:some/property]}]))

  then Compassus will ask your parser to read the query:

  [{:app/home [{:some/data [:some/property]}]}]

  If you don't care about the route key, and you'd like your parser to
  treat :some/data as a root-level key in the query, wrap your parser in
  flattening-parser. Your parser will instead read the query:

  [{:some/data [:some/property]}]"
  [parser]
  (om-next/parser
   {:read (fn [{:keys [query target] :as env} key params]
            (if-not target
              {:value (parser env query)}
              {target (let [subquery (parser env query target)]
                        (when (seq subquery)
                          (parser/expr->ast {key subquery})))}))
    :mutate (fn [{:keys [ast target] :as env} key params]
              (let [tx [(om-next/ast->query ast)]]
                (if-not target
                  (let [{:keys [result ::om-next/error] :as ret}
                        (get (parser env tx) key)]
                    {:value (dissoc ret :result ::om-next/error)
                     :action #(or result (throw error))})
                  (let [[ret] (parser env tx target)]
                    {target (cond-> ret
                              (some? ret) parser/expr->ast)}))))}))

(def parser (flattening-parser (om-next/parser {:read read :mutate mutate})))

The parser that flattening-parser wraps behaves the way I believe you'd like, and the one it returns behaves the way Compassus likes.

This is the behavior that I think Compassus should optionally provide. I think providing it like this, along the same lines as compassus-merge, would be a good way to do it.

There's another wrinkle when it comes to remotes, though: the remote query is still wrapped in :some/route. Currently there's no way around that without modifying Compassus, because Compassus only supports a single key in the root of the remote query. We use query roots and om/process-roots to deal with that, but it would be nice to have Compassus support a way that doesn't require that.

molst commented 7 years ago

Thanks for making this a lot clearer! Using this function took me closer to what I was expecting. And you're correct, the remotes wrinkle is there although it seems manageable.

anmonteiro commented 7 years ago

I'm ready to work on this. The problem I'm facing is:

Feedback very appreciated!

anmonteiro commented 7 years ago

One idea would be a :parser-type key in the configuration map which would default to the current behavior, but could be changed to :pass-through or whatever.

One benefit of this is that it would allow for future backwards compatible changes if we come up with another behavior for the parser.

Peeja commented 7 years ago

I'd love to be able to just refer to the parser builder (of my choice) directly, much like how you can choose to use compassus-merge. I'm not sure there's a great way to do that that's backwards-compatible, though.

That would mean that the user could build their own complete parser to hand to Om if they really wanted to, or other libraries could theoretically provide alternatives. Feels nicer to me than providing a closed set of options.

anmonteiro commented 7 years ago

fixed https://github.com/compassus/compassus/commit/8db377181842674334914ccf51fba0ab94e271c0.

Also fixed the dispatching on a single remote key.