Transactions without quoting

[thheller]

One thing that really bothers me about the transaction API is the quoting required to qualify the symbols.

I have an alternate proposal that would only require a tiny modification to the defmutation macro.

The use doesn't change

(defmutation the-mutation [...]
  (action [...]))

but when constructing a transaction the user could do

(fp/transact! this [(mut-ns/the-mutation {:params foo})])
;; instead of
(fp/transact! this `[(mut-ns/the-mutation {:params ~foo})])

defmutation could either just create (defn the-mutation [params] (list 'some.mut-ns/the-mutation params)) or create a type/record instance which implements IFn.

(deftype TxDefinition [tx-id]
  IFn
  (-invoke [this]
    (list tx-id {}))
  (-invoke [this args]
    (list tx-id args)))

;; emitted by the macro (in addition to the multimethod)
(def the-mutation (TxDefinition. 'some.mut-ns/the-mutation))

(the-mutation {:params 1})
;; -> (some.mut-ns/the-mutation {:params 1})

The deftype/defrecord solution is slighty more extensible since it could also implement other protocols and take on more responsibility like spec-validating the passed params when called.

The quoted version would still work without modifications so it would be a non-breaking change.

This would also works nicely when the mutation impl is actually defined in other namespaces.

(ns my.app.api
  (:require [fulcro.api :refer (deftx)]))

(deftx the-mutation
  "docstring"
  param-spec)

(the-mutation {:params 1})
;; => (my.app.api/the-mutation {:params 1})

(ns my.app.client-impl
  (:require [my.app.api :as api] ...))

(defmutation api/the-mutation [...]
  ...)

(ns my.app.server-impl
  (require [my.app.api :as api] ...))

(defmutation api/the-mutation ...)

The main purpose is to get rid of the quoting for transactions. I can open a PR if interested.

[awkay]

So we just started a fulcro-incubator project to play with ideas like these. Wilker had some great ideas around additional "lifecycle" methods for transactions that he's already placed in there. The current macro was created to solve really just one problem: IDEs and tools don't deal well with multimethods. With defmutation you get something that you can treat sorta like a function, and the tools can then navigate to (and show docstrings for). It also helped reduce errors with returning a map (e.g. ppl would often write their actions in the body, which looks to work ok, but isn't right at all).

I'm game for extending the macro features over time, but I'd prefer we make a sample implementation in incubator first, with appropriate docs. That way we can gain some experience with them before making changes to the core API that cannot be "undone".

I have experienced the "quoting" pain, so on the one hand I'm actually quite attracted to your idea. On the other, the quoting makes it unquestionably obvious that txes are data, and not actual function calls. This new notation blurs that line and makes them look more magical, so I don't really like that about it.

Another approach I've thought of is making a macro called something like invoke (that is just a glorified call to list that quotes the symbol) that could be used to eliminate the quoting...

(transact! this [(invoke mutation-name {})]

That would be much less behind-the-scenes magic, and would be trivial for ppl to jump to and understand. Of course, you can still quote if you don't mind quoting.

[thheller]

I just pushed my first shadow-cljs UI preview with some of the fulcro modifications I mentioned above. In particular I created the basic deftx macro that create the little transaction helper records.

The idea is that you'd have a dedicated namespace [1] just just declares which transactions you support and the params they accept via spec or so (the actual macro above doesn't do anything with those yet though).

https://github.com/thheller/shadow-cljs/blob/daf9e3bd2ff4d8ffee36354cbc84012333482fd9/src/main/shadow/cljs/ui/transactions.cljs#L14-L16

The var created by deftx is actually callable but only returns the proper mutation data. So calling (tx/build-compile {:build-id :foo}) just yields the (shadow.cljs.ui.transactions/compile {:build-id :foo}) (and would validate the passed params in the process).

https://github.com/thheller/shadow-cljs/blob/daf9e3bd2ff4d8ffee36354cbc84012333482fd9/src/main/shadow/cljs/ui/app.cljs#L55

defmutation also was a bit too macro heavy for my tastes so I opted instead to add a simple helper fn instead. defmutation still does way more but I added everything I needed so far to my fn.

https://github.com/thheller/shadow-cljs/blob/daf9e3bd2ff4d8ffee36354cbc84012333482fd9/src/main/shadow/cljs/ui/app.cljs#L185-L187

I'm a total fulcro beginner so this all might not make sense at all but I really disliked the quoting so I had to get rid of it first. I like the "interface" nature of the deftx and that I can easily jump to its declaration/usages in Cursive. My namespaces currently are a total mess but all the deftx should actually be shared with the server as well at some point. Currently there just refer to their symbol [2] which kinda defeats the purpose.

[1] https://github.com/thheller/shadow-cljs/blob/daf9e3bd2ff4d8ffee36354cbc84012333482fd9/src/main/shadow/cljs/ui/transactions.cljs [2] https://github.com/thheller/shadow-cljs/blob/daf9e3bd2ff4d8ffee36354cbc84012333482fd9/src/main/shadow/cljs/devtools/graph/builds.clj#L78-L88

[awkay]

Thanks for sharing. When I get a few spare moments I'll comb through that and try it out. I appreciate you pitching in and trying to make things better :)

[awkay]

So I looked it over. Here are the things I like:

• Support for specs. I'm not terribly sure how much they matter since the call tree of mutations is always so trivial, but I at least like the principle.
• The reduction in boilerplate over a common case or two (e.g. not needing to be explicit with the swap all the time)

Unfortunately, it does come up short in several ways, and if you don't like the macro the multi-method is public and open to extension, though as you say in your comment: overriding the default isn't the sanest thing to do...why not just call add-method (or I guess defmethod, which calls add-method)?

The reason we want the multimethod is for libraries. If some 3rd party fulcro library wants to give you some mutations, there's a very standard place to do that: as a FQ symbol on the multimethod. The macro exists because I wanted better errors for users. The multimethod has to return a map with special keys (which your method returns to), and that was the source of continual mistakes and errors for both beginners and experienced users. The macro helps guide people to write them correctly, gives compile-time errors when they make mistakes (instead of head scratching at runtime), and gives them a clear Cursive jump target. I personally use it most of the time, and it works very well. I occasionally drop to using the multimethod directly on those occasions when I have some common logic between sections (e.g. a let that declares something useful to both the action and remote). This case is pretty rare, but does occur.

After adding defsc and defmutation as error-checking macros a whole lot of questions stopped happening, and my own experience using them has been quite good. I personally think macros are great for exactly this sort of problem: a common repeated pattern where users make mistakes you can easily catch and correct at compile time. I also understand that experts sometimes don't want the handholding or magic. That's why the underlying multimethod is open and not an implementation detail.

I'm glad you found a pattern that is working for you, but I think the current implementation is quite good for the main user base. I agree with you on the quoting, though. I may add the invoke (possibly under a different name) to incubator and play with it.

[thheller]

Uhm you misunderstood my intent here. I should have been more clear. It was absolutely only about being able to "call" a mutation to create its data representation. This could be done as part of the defmutation macro.

(defmutation the-thing [params]
  (action [...] ...))

(the-thing {:id 1})
;; => (whatever.ns/the-thing {:id 1})

I like this over the proposed invoke macro since it keeps the mutation looking like its data form. The standalone deftx was only about being able to declare mutations independent from their implementation as kind of like an "interface". It is however optional.

The rest is just me exploring some stuff while learning. I absolutely understand the purpose of the multi-method and why it makes sense to use it. Ignore all my own thoughts on that subject.

[thheller]

You can see the minimal changes required for the ultra-simple version here: https://github.com/fulcrologic/fulcro/compare/develop...thheller:callable-mutations

(ns the.thing
  (:require [fulcro.client.mutations :refer (defmutation)])

(defmutation something [params]
  (action [env]
    (prn "called something")))

;; note: no quotes, constructs mutation form for use in transact! only
(prn (something {:id 1}))
;; prints only
;; (the.thing/something {:id 1})

This minimal modification you already gets 90% of the desired result.

[awkay]

Ah, well, thanks for clearing that up. That is actually better. I hadn't done the invoke because I felt like there had to be something better (or quoting was just ok)...I like it.

It also doesn't seem to break anything. If you quote it, you get the existing behavior. If you don't quote it, you get what you "mean". The only thing I'm concerned about it is that when you're forced to quote, you at least understand a mutation is just data...but I can address that with documentation improvements.

I wonder if @wilkerlucio or @currentoor have any thoughts on this one? It seems like a good add to me, and I don't see how it could break things.

[thheller]

There are a few issues that would need to be checked. I don't know how common these cases are but it looks like (defmutation some.ns/foo ..) is valid (meaning namespaced syms)? That would cause a (def some.ns/foo (MutationHandle. ...) which is not valid.

There is one conflict in fulcro routing where set-route is re-declared but AFAICT that is old and not used anyways.

Besides that it should be fine.

[awkay]

Ah yes...a fully-qualified ns is allowed on defmutation. I guess we could document that using explicit namespaces on defmutation disables this feature and causes you to need quoting, and perhaps even emit a warning. It's an unfortunate artifact of how things evolved, and I personally strongly discourage placing a mutation in a namespace different from what the client calls. There's just no need for that disconnect.

I guess we could also technically push the symbol into the correct namespace, though I don't like manipulating vars in other namespaces...sounds like a recipe for conflict/disaster.

[thheller]

Yes, manipulating another namespace would be an absolute disaster (eg. breaks caching). Just skipping the added def when sym is namespaced sounds fine to me.

[wilkerlucio]

I like the idea, but I found one incompatible thing. Currently, the defmutation supports the ^:intern flag, which creates a function that runs just the action, I have used this a couple of times when I want to re-use the action in another mutation, this is convenient. That said, I don't think many people use it, but it would be a break for people using ^:intern. Having ^:intern + invoke could be confusing as hell, because users will not know what the function is returning (a list or calling the action?).

We could remove ^:intern entirely so it's not confusing, but it's a breaking change.

[awkay]

@wilkerlucio I saw that, but I don't think it's a problem. It won't break existing apps to leave it as-is, and we can just add docs to the intern metadata saying it doesn't obviate the need to quote. I mainly added it so devcards could find the source of mutations. I really have switched to writing helpers that can be composed into swap! as the primary pattern, so I don't think it that useful to have intern anymore (esp now that I'm not using it to find source since I stopped using devcards for docs).

I'm perfectly fine with deprecating intern, and just saying it doesn't auto-quote.

[awkay]

I see another thing I don't care for: It is often the case that mutations need to require UI components, that in turn would like to require the UI component (for example to use it in a merge). Right now, this is easy to explain, because the mutation is just data and the quoting makes that obvious. If we change defmutation and advertise that "quoting is no longer necessary", I think we need to be really clear where the quoting still comes in handy.

[wilkerlucio]

@awkay I didn't understand the require UI component bit, I understand we sometimes want to use them in the mutations, but not seem the relation of that with calling the mutation. can you give a code example?

[awkay]

If you want to use merge-component in a mutation, you need a component to supply a normalizing query. It is possible that in that same file are mutations that are used by that component. Circular require. Technically, since mutation names are just data, you need only require the component in the mutations file, and not vice-versa (since you can quote a literal symbol); however, with this new scheme you get quoting avoidance and name aliasing by requiring the mutation file, which will make it even more desirable (though still not required). The other workaround is to co-locate those mutations with the components, but that isn't always very clean either...then your mutations can end up peppered in strange locations for bad reasons.

Ultimately the problem is a limitation of the language...but it is annoying nonetheless.

[thheller]

Thinking about it a bit more I really don't think that combining this with defmutation makes a whole lot of sense because of the coupling it promotes. Right now the Component only needs to know the name of a mutation and I guess the :require for the mutation ns is only done out of convenience for the quoting. If the require is done for an actual purpose that couples everything more and introduces the circular dependency Tony mentioned above.

I still think that there is value to being able to declare a mutation independently of its implements like I have done in my UI stuff but my experience with fulcro is so limited that I may be changing my mind on that subject again too.

This sounded like a nice easy addition but I'm not sure the extra "enforced" coupling is worth it.

[currentoor]

This does look nicer and makes it slightly more convenient to use. But as the library grows, the cost of supporting little things like this can really add up. I'm not totally apposed this (not a strong advocate for it either).

I really like @thheller's idea about separating mutation interfaces and implementations. Given the language constraints, that seems like the ideal solution. Like if Fulcro required you to declare all your mutations in one place, similar to bidi routes. I think that would help encourage better API design because when you add a new mutation you're forced to consider how that mutation fits in with the rest of your API. Compared to what happens organically, some mutations clumped together in an API namespace and some peppered in the component files.

[awkay]

Yeah, I think I prefer the simplicity and consistency of just requiring people to think about and treat mutation names as pure data. Another option is to add :autoquote to the supported metadata markers on a mutation. That would make it opt-in, but then people that like it would consider it a hassle and would whine about it not being the default.

Another option to avoid the quoting would be to make transact! itself a macro that would pre-parse the transaction..but that's getting more magical and mysterious than I like. The fact that mutations are data is a critical part of how you should think about Fulcro, and I'd prefer not to hide it.

[currentoor]

If you separate interface from mutation and make the interface callable (where it returns (some-ns/mutation-name ...)) you get

• docstrings in all repl connected editors (not just intelliJ)
• auto complete (by referring the interface namespace in component namespaces)
• no circular require issues (the implementations can reference all the components they want)
• no need to quote
• logically place mutation implementations that require components, rather than peppered in UI namespaces
• I use a lot of idents with namespaced keywords, with this approach I can use aliases instead of the whole ns in keywords
• putting the interface in one place encourages holistic design such as consistent naming conventions and noticing patters like “these 3 mutations do something similar, may they should be combined”

For example, in my app some mutations had as an argument, :id and others had :package-id but using :package-id everywhere meant I could reuse my authorization logic so I had to refactor, I think if I declared the interface in one place, with argument specs, I might have called it :packaged-id everywhere.

[awkay]

So, I'm glad to take contributions to address this in incubator. I think the improvement possibilities are worth exploring (getting rid of quoting and circular references would be a nice win). I don't like that jumping to declaration would then take you to the interface...that is probably the feature I most use, but I guess if the real symbol was used right there, you could just move to it and jump again.

[awkay]

After sleeping on this a couple of days I decided to reopen this issue. I think this deserves treatment in the developer's guide, and possibly some kind of API additions. I played with some approaches in code an I generally like the idea of a deftx that does what is suggested (lets you declare the transaction separate from the implementation). I think we have good reason not to make that part of defmutation, but I think there is an equally compelling reason to make it possible for people to declare the mutation separately with deftx which actually lets them solve significant problems including "circular referencing for the convenience of aliases" and "quoting", along with those listed by @currentoor .

At the moment, it seems like we want:

• [ ] A deftype (as shown earlier) that supports invoke to return the mutation call itself.
• [ ] A decision on what we should write for declaring them. Is (def my-tx (Tx. 'mutation-sym)) sufficient, or do we want more?
• [ ] Updates to the developers guide.

Comments?

[thheller]

FWIW I don't like the deftx name anymore because it describes a mutation not a transaction. Can't think of anything else though given that defmutation is already used.

I do think it is useful to co-locate some kind of validation for the mutation params within the deftx and a docstring but nothing beyond that.

(deftx build-watch-start
  "(shadow/watch build-id)"
  {:build-id keyword?})

This reads nicely and you can quickly tell at a glance which params you are supposed to pass for the given mutation. My impl currently completely ignores the spec but the idea is to validate the passed params on call so that it fails as early as possible (before calling transact!). The validation could also be completely elided in release builds. I'm currently using pseudo-ish spec. It could just take actual spec however (eg. s/keys).

As far as the defmutation is concerned it could maybe try to resolve the passed symbol and if it resolves to a var created by deftx take the symbol out of that. Don't know if that would break existing apps though since currently nothing is resolved and it is fine to pass tx/foo.

[currentoor]

FWIW I'm happy with deftx, since the Fulcro API function it gets passed to is called prim/transact!. Mutations are ultimately transacted. Each individual mutation snippet, (some-ns/foo {...}) is a tx and a sequence of them [(some-ns/foo {...})(some-ns/bar {...})] is a txn or transaction.

I use a very similar naming convention with datomic transactions. If a function returns [:db/add id :foo/bar 42] then the function name is foo-bar-tx. If a function returns

[[:db/retract id :foo/bar 42]
 [:db/add id :foo/bar 64]]

then the function name is foo-bar-txn.

I agree with everything else @thheller is saying 👍

[awkay]

So, I've been coding this way for a bit (separate "interface" vs defmutation). I'm not loving it, but that could still be my problem adapting to the extra work:

1. The extra declaration in a separate interface ns (to fix circular possibilities).
2. Code navigation then requires multiple jumps to get to the actual thing
3. It requires discipline to do every mutation this way...and ending up with a mix of both makes it really confusing as to which should be used.

I thought about the automating it with defmutation, but since that does not fix the circular ref problem I find it less compelling, and I have not idea how to do the magic to even make it work: Clojurescript doesn't have vars @thheller, so I'm not sure how I'd go about automating the defmutation doing it only if there isn't something already...well, and it's a macro, so it runs at compile-time in the CLJ JVM, so I guess I'd have to play with the analyzer??? And what if the "conflicting" declaration is just below the mutation? Is the analyzer multi-pass where I'd still be able to find it?

I guess I've come to the conclusion that including the idea with some implementation in the dev guide might be the current best course. Throwing a sample implementation in incubator might also be useful.

I tend to prefer syntax that works well in the IDE...so, if there was a macro I'd want it to probably "look" like a defn so that the IDE could report the parameter list and docstring in a way users would appreciate. Like (defmi symbol "docstring" [{:keys [boo bah]}] (s/keys :req-un [::boo ::bah]))...naming is so hard. declare-mutation?, defmi (mi = mutation interface)?, ???

[thheller]

@awkay you can use (cljs.analyzer/resolve-var &env some-symbol) in macros to check how a symbol would resolve (eg. foo/bar -> my.app.transactions/bar) . This has the same rules as normal Clojure code so a var has to be defined before defmutation can access it. No the analyzer is not multi-pass.

Incubator seems like a good place for this. No clue about the name.

As far as my experience goes I still like declaring the mutation "factory?" separately very much as it get rids of the quoting and the coupling to the implementation. Clicking twice to jump is fine IMHO since it doesn't need any special IDE support at all. I have not been using defmutation as well though.

So far I have been doing everything in one file at the start and then slowly moving things to separate places/files when things get bigger. Given that the mutation interface declaration is already separate I never have to worry about how to organize the files since circular deps simply don't happen.

[awkay]

Added to incubator, and to Developer's Guide.