Fix templates and book to recommend Pathom setting for always returning :tempids

[holyjak]

Add :tempids to mutation join queries by default so that all users do not need to do that themselves (and '* if there was no query before to preserve Pathom's behavior of everything the whole mutation output if the user did not ask for anything in particular).

Add ::p/errors to all queries and mutations - most users of Fulcro use Pathom and need this and an extra keyword will not harm those who don't (and they can always remove it via a custom global transform).

Also expand the default elision list to RAD's (for the sake of simplicity, include also the *.rad.* ones, they are just keywords so do not create any dependency on RAD and will make its life easier).

Consequences

• Many users likely do some of this in their global eql transforms. If they combine their with Fulcro, they might end up with these props twice in the query, but that should not harm anything, since we get back just a map, so no duplication there
• Users will far less run into problems where tempid remapping does not work, b/c they used a mutation join and forgot to add :tempids
• Users will get pathom errors back by default, which will make it easier to discover what went wrong with their query/mutation

[awkay]

This is close to what we want (if we expand the default exclusion list):

(defn global-eql-transform
  "Returns an EQL transform that removes `(pred k)` keywords from network requests."
  [pred]
  (fn [ast]
    (let [mutation?     (symbol? (:dispatch-key ast))
          has-children? (seq (:children ast))]
      (cond-> (-> ast
                (rad-app/elide-ast-nodes pred)
                (update :children conj (eql/expr->ast :com.wsscode.pathom.core/errors)))
        (and mutation? (not has-children?)) (update :children conj (eql/expr->ast '*))
        mutation? (update :children conj (eql/expr->ast :tempids))))))

[holyjak]

Some questions, @awkay, to determine whether adding ::p/errors anywhere to transactions with mutations is meaningful or not - because it seems that errors of a mutation itself is returned as {<mutation-symbol> {::p/reader-error "some text"}}.

1. Is it so that a "query" and a mutation is never combined in a single transaction / request to pathom, i.e. if the transaction contains a mutation, there are never any non-mutation top-level joins or properties - something like [:x {:y [:z]} (mutation)] will never happen and thus adding ::p/errors to the top level is pointless?
   (though in RAD there is perhaps <mutation symbol> :com.fulcrologic.rad.pathom/errors?)
2. Since mutation errors are returned as reader errors and are returned by default, we do not need to add anything to the transaction to get them in Fulcro

(For mutations, we could perhaps offer a function you can hook into the app's :remote-error? that would mark mutation reader-errors as errors so that they would appear in the component's ::m/mutation-error, as produced by the default result action?)

[awkay]

Honestly I do not remember/know.

In terms of what needs to be in the query: remember that if there is a mutation join that Fulcro uses that for merge as well, so if the key is not in the outgoing query Fulcro itself will filter it.

To be honest I'm torn on how much pathom-specific stuff to include, since that is an external lib that is in the process of changing how it works. I'd probably tend to leave the pathom-specific keys out altogether and document that you might want to customize that part for your app.

[holyjak]

No need for :tempids in mutation queries?!

According to my experiment, we do not need to add :tempids to mutations:

1. If the mutation has no query then Pathom simply returns everything by default, thus including :tempids
2. Even If I add a query via m/returning - and the server-seen query becomes [{(com.example.mutations/create-random-thing {:tmpid #fulcro/tempid["1b59713c-305c-4b8d-802d-7b39b8a4e696"]}) [:fake :com.wsscode.pathom.core/errors]}] - still tempids are returned to Fulcro

(Pathom 2.3.1, Fulcro 3.5.2)

[awkay]

Hm. I have seen this mess up in RAD, so I know it doesn't work through the stack. It could be that Fulcro's the one filtering the tempids? Does the remap work in case (2)?

[holyjak]

It does. I will try this with RAD demo itself.

[holyjak]

Confirmed that in RAD Demo I need to add :tempids to mutation joins otherwise tempids are not returned by Pathom. Need to explore.

[holyjak]

Oh, I see why getting back tempids worked in my first test. I had this in my parser config:

{::p/env {::pc/mutation-join-globals [:tempids], ...}}

@awkay wouldn't adding the Pathom config ☝️ be a simpler solution than modifying the global-eql-transform in Fulcro? It would require more work from the user, namely to copy a "recommended" Pathom setup - but I expect that most users do that anyway? But it seems that neither fulcro-template nor fulcro-rad-demo do this for some reason?

Above you wrote

if there is a mutation join that Fulcro uses that for merge as well, so if the key is not in the outgoing query Fulcro itself will filter it

Does that mean that is :tempids is not in the outgoing Fulcro query then Fulcro will ignore it, even if it is in the incoming data? But that is not congruent with my experiements so maybe tempids is special, compared to any other query property?

You also wrote

I'd probably tend to leave the pathom-specific keys out altogether and document that you might want to customize that part for your app.

I guess it means you would after all prever not including ::p/errors in the query, is that correct? If that is the case - what about using a general, Fulcro specific key, such as com.fulcrologic.fulcro.networking.[http-?]remote/errors, and expecting that the Fulcro server-side glue (or the remote implementation) remaps that to whatever the particular kind of remote uses? So in the case of Pathom, it would be some fulcro-integration-plugin that would move ::p/errors from the result into this key on the way out?

[awkay]

Yes, I was not aware of this option in Pathom. It would be better to just recommend that and fix the templates.

In terms of the errors: Fulcro does not require Pathom, and how you deal with errors on Pathom is also pretty configurable. I'm fine with setting up the templates to glue together error handling in some "useful" manner but given that Pathom 3 might even change this key I really don't want to chase Pathom on this in Fulcro's code.

[holyjak]

Then I guess we should close this issue as "won't fix"?

[awkay]

Yeah, let's open up the templates and fix them, and also document it in the tempids section of the book. Let's just track it with this issue, since all the history is here.