Whohoo return values for mutations (will be very useful for certain situations)

Hey everyone trying to use latest cljs: @wilkerlucio just sent me a PR that fixes Untangled to work with the latest Om/Cljs. I have applied it to develop branch, and pushed it to clojars as part of 0.6.0-SNAPSHOT

@tony.kay just fyi, I just tested compiling with the latest snapshot and it's working again, thanks for the release 🙂

welcome

thanks for figuring out the fix

I'll probably cut an official release soon

hoping to get the forms stuff worked out as well this week. Too many things going on

I'm really excited about the forms addition.

nice, exciting times

I also need to get some cookbook recipes and docs around the new additions

if anyone is looking for stuff to do 🙂

@tony.kay what features you are looking for help on documenting?

The stuff in the changelog

@wilkerlucio like the new return value support

want to put it in the devguide

probably using a client-simulated server, which I've done in other files.

post-mutation parameters can probably just go where post mutations are talked about

Make sure the new load function is in data fetch, and note the deprecated load-data.

those are the main updates that are needed

Then, of course, I want to start a section on forms when I get that done

I would also like a section covering the i18n story

and one for the support viewer

both of those last two need cookbook examples too

@tony.kay ok, I'm still trying to catch up with all the new goodies, I would like to experiment a bit with the server-return, then I can help with some docs, do you have an example use case for the server return?

Yeah, um....where did I put that 😉

@wilkerlucio See the discussion above with jasonjckn (11:11am ysterday)

it has pointers to the exact lines of example code in my template

and a discussion of how/why. I made one change at the end of that discussion, so make sure you follow that 🙂

I hope it didn't throw off the hyperlinks

My coding guidelines around it are to make your return handler a multimethod, and co-locate the return handler with the mutation.

that way you can reason about them together

If I had control of the Om parser internals I probably would have made it an additional key on mutation methods (:value, :action, :return-action)

@tony.kay just to check if I'm getting it right, the :return is something you can get back from a server mutation, and then it will be dispatched into a post-mutation to be handled on the client, is that correct?

@tony.kay perhaps the mutation results should trigger a mutation ? it seems like the untangled api is asymmetric, e.g. on mutation failure (tx/fallback) => trigger mutation, but on mutation success (mutation result) implement some global handler

in that bucket we also have post-mutations

@jasonjckn the problem is we only have the symbol that is already an Om mutation

and made the original net request

we need a separate entry point

e.g. here's symmetry: post-mutation: a mutation triggered to operate on recently changed local state mutation result: a mutation triggered to operate on recently interim state returned fallback: a mutation triggered to operator on error state

yes, but fallback is for things like server exceptions

return values are for...well, anything

and each mutation may want a specific handler for its return value

yah good points, oh well, i guess it's the best design right now, in theory, you could add (tx/mutation-result :action 'foo/bar)

but i don't know if that improves anything

i guess i can feed mutation results into another multi method

yeah, just makes it noisy

if you co-locate the defmethods of mutate and handle-return, then the code is all right together

that was my comment earlier about guidelines

I suppose the 'real' API win would be the following:

(defmethod mutate 'foo/bar [..]
  {:value {:keys []}
   :action (fn [] ..)
   :result-action (fn [] ..)})

is that workable?

I wish

that was my earlier comment

see above about 1 hr ago

kk

If I had control of the Om parser internals I probably would have made it an additional key on mutation methods (:value, :action, :return-action) ah yes

yep

didn't read that till now

so we're on the same page, too bad that's not doable

but multi method should be fine

well, we can fork Om 😉

om.next.next

😛

nice

maybe it's worth opening a githbu issue about this

you won't be the only one to try and wrap om.next in a framework

on Om?

I can already tell you the answer

yes to see if they can make it more pluggable

The answer is to customize your own merge. Hm.

my brain is itching

in clojure philosophy data is extensible, e.g. {:untangled/mutation-result ... :action ... :value ...}

which means there may be a way to hack that in after all

interesting 😄

actually, I think there is

darn it...

OK, well, thanks for pushing on that. I see how to do it now

@wilkerlucio might want to hold off on docs, I think we can do without the extra mmethod

i love it when we our conversations lead to these refinements 🙂

me too

it's also why some of this stuff is in a SNAPSHOT

play with it

also may be worth doing :fallback-action in the same place if that makes sense, you could keep both apis for backwards compatibility.

fallback is needed for exceptions. Don't know what went wrong, so they're not as easy to target

kk

they are the fallback for everything in the tx, not just an item

ah yes

if you want targeted, use return vals

:result-action or :return-action?

so the arguments are are [env k result-without-tempids] ?

hm. This might not be the best idea, for other reasons. I have no AST or anything. So, if you write a mutation that wants to play with that stuff globally (e.g. to rewrite the params of a remote) it might crash

in other words all I have for env is state

hm

I could mark it in env

(:is-return? env)

that seems fine

in theory you could pass the right env, you just don't know how to do build it up yet

leave it for another PR

it's just not easily available to you through om.next, the env still definitely makes sense in mutation results

-that- might be something to open a PR about in the future

I'm not sure I can't get to it...I'll just have to walk through the implementation

for now is-return? is good enough

but a marker seems good nevertheless.

i'd leave that for another day

honestly nobody will probably notice

can you put the right :ref in there though?

i like :remote-result fwiw

or possibly (:remote (assoc ast :action-result ...) :action ...)

@jasonjckn erm, :result-action?

it's not a result, it's an action

it's all okay but that name doesn't really highly it's the remote's action, not the local action

bleh...naming

there's really 2 different actions, easy to conflate

:remote-result-action

wordy

yes there's no confusion there

yah wordy

(:remote (assoc ast :action-result ...) :action ...) is more in line with om.next

but not as 'easy'

yeah, I kinda hate that

🙂

yah i wish om.next did something else there

for situations where you have remote params

:remote-result ?

no?

I'm sorta liking how it is currently implemented, partially because the env isn't the same (since we're no longer parsing a query)

I think conflating them is going to cause all manner of confusing bugs

but I do like the symmetry of this new approach

the env is the same no? it's just we're not sure how to generate it right now

Let me dig in the plumbing for a min

here's the problem: our hack location is in Om merge

and Om does not pass the original env of the request to me

I can get to app state because I made it

so I can close over the atom in the merge routines

All I get from Om is the target and source values

and you want to be called after all of the merging is done...so this is the phase to run in

um...I can probably hack it through with metadata

but to get env absolutely correct I'd really have to hack into the parser

is there some conversation you can have with @anmonteiro about this

and re-parse the original complete tx while dispatching to this

some new extension to om to make env available

no, this is something Om is leaving up to the implementor for good reasons, I think

changing the parser is no big deal IMO

that's how om.next has meant to be used, every tutorial is like that

I'm kinda leaning towards my current solution again. I don't think this is something ppl are going to need a lot, and co-locating a clear mmethod next to your mutation really is pretty clean

no, you're wrong there.

we use the term wrong

our multimethods are really emitters

the parser is the thing that makes an AST

and dispatches to our emitters

I'm saying we'd have to hack the thing that makes the AST and dispatches

i see

which is not something you ever do

yah that's no good

I can see a hack that would work, but it is uuuuugly

mmm, not sure it would work either

does it make it easier if it's {:remote (assoc ast :action-result (fn [] ..) :action } this seems like the right place for it, does this not give you some flexibility in the remote implementation to call the action-result handler when the remote action finishes?

no

well

hold on

not seeing how that could work

and then I've got a mess of logic to even pull it out

in a whole different part of the plumbing

the problem continues to be the correct env, which I cannot generate

but your server code can pass back anything you might need

which is the intention

e.g. you can add things in your params on the mutation that you want reflected back

yeah, I'm sticking with how I've implemented it. It's just too fragile and complected to add this into mutate, even though on the surface it looks like a good idea

untangled implements this remote right? so in the implementation, here's a shitty one, but you maintain a registry of mutation name->{action-result function , env, etc} then when merge comes back (or maybe there's some callback in remote) you use that registry, obviously this sucks, does it work? i'm not that familiar with om, but i feel like if it's doesn't already support this 'callback when remote mutation comes back' this seems like a logical flow of it's design

It seriously makes an internal mess.

for very little benefit

sure I follow

anyways if you ever think of some small change to om.next that would allow this to be internal niceness, definitely post an issue

not remotely simple to accomplish or maintain

i don't know the specifics of om, but just thinking about it generally it seems to flow at least in my mind, so maybe there's some small change they can make

whether that's env available in reconciler pluggable merge, or env available in remote mutation ajax cb, etc, etc

env is generated during the parse, not any of those other phases

the whole constructions of invocations of parse has to do with the front-side of transactions, not the back-side

the results of parsing, e.g. the AST are needed both before remotes are contacted, and also after they're contacted, to for example normalize the returned data

so from that abstract view it would follow env should be available after remote returns with action-result

i understand the specific om.next api right now doesn't allow for that, but maybe there's a small change worth suggesting

hm. There's actually a trick we could leverage that is already supported

hadn't thought of that...

😄 that sounds promising

so, when you invoke the parser, you say (p q) or (p q :remote)

the :remote is convention

multiple remotes just use diff keywords

(p q :result-action) would act like a remote parse

yess

oh, but it would expect the value to be a boolean or ast

hm

not a thunk

well you could do the (assoc ast thing

are you the first person to use this for non-remote things?

I don't think you should, actually

yah good brainstorming, but i think we're back to where we were before

{:result-action (callback body)}

right

where callback is a macro that emits an ast hack

are you the first person to use :remote for non remote things?

I don't know of anyone even thinking along these lines, no

wouldnt callback need env or ast?

it would

yes, but I'd call my own parser on the original tx

ah so it doesnt need to be passed

you'd just use it from the param list of your mutate

which I'd be invoking again

close over it

it's really hacky to get a small amount of syntactic sugar

agreed

talk to @anmonteiro at some point, I don't see why there isn't a small change here in om.next

well, perhaps we'll find future creative things to do that need these ideas. I'm going to stick wit hthe current impl

It really isn't a small change. The ability to plug-n-play your own database, networking, and everything else eliminates being able to do it well, I think

it's very clojury to support {:action ... :remote ... :untangled/action-result } sort of thing and there's no reason why env shouldn't be available, it's needed to merge&normalize, so why not for :untangled/action-result

it is not needed

the query is needed

and you can modify the query before you call merge

actually merge doesn't even use the query

normalize does

and that certainly isn't a place to be calling callbacks

you haven't even put the data in the db yet

Q -> net -> server -> resp -> om callback -> normalize -> merge -> result-callbacks

ok, and you don't like on-success mutation? even though you have post-mutation and error mutation

post-mutation was experimental, and I don't think it should remain, to be honest

ok

what error mutation?

tx/fallback

but that just is a hook to trigger mutations

on-success is what I just added 🙂

and it needs to trigger on the original mutation symbol

nods

alright well looks like you chose the best available option then

For now. It is almost certainly possible to make our desired thing work

and it will be an easy port from the current implementation

so, let's call this v1 of return value handling and move on

nods

jokingly just write a defmutation macro that looks like what you want but outputs 2 defmethods

haha

Actually, I kind of like that best so far

yah i mean i'll probably just do that for the team here if that's the api lol

i do want the logic co-located

but how often will you acutally need it? I've written a lot of Om code so far without needing it at all

I can see the basic need, but it seems like something you rarely should use

not often, but the other guys keep asking about it

yeah, but part of that is a desire to do things "the old way"

and should be discouraged 🙂

for sure, i've had the conversation a number of times now

yah definitely

optimistic updates -> tx and fallbacks on the (very rare) failures is soooo much better for the end user

and the developer

it's not just with developers though, it's also with UI people

(the conversation) 😞

The UI ppl should not even be aware of a remote

their little spinners and stuff

except for lazy loading

yeah, but that is loading, not mutation

well action buttons with spinners

"applying action... " "action applied.." sort of UI feedback

anyways i'm on the same page as you