Okay, so I got all of that working, and now I have the same bug again - trying to lazy-load data by a temp-id fails

(specifically this error: https://github.com/untangled-web/untangled-client/issues/9) The outgoing read request does the right thing - it doesn't query a tempid, so I think this is specifically an issue with how load-field-action figures out to merge the data back in.

Looks like the closure here holds onto the tempid in a way the rewrite-tempids-in-request-queue can't rewrite: https://github.com/untangled-web/untangled-client/blob/b0814d0132b2bee2f7173bea76a6385c6e9cb952/src/untangled/client/impl/data_fetch.cljs#L50

I'll see if I can figure out how to fix it tomorrow.

@therabidbanana: Ah. Thanks for tracking that down. However, you should still not be submitting a read against a tempid in the first place. What is the sequence you're trying that is even ending up asking for soimething by tempid.

The sequence I want to have is: [(widget/new ~params) (widget/load-data-stream)]

The data stream takes a while to load - and we lazy load them only when widgets are brought up on the screen. So yesterday I refactored the lazy load to be a mutation called (widget/load-data-stream) so that we can lazy load them when we call (dashboard/open)

That works well, but now I'm encountering the case that when you create a brand new widget, we want to queue a request to start fetching the data, lazily

ah, but the new is a remote thing that uses a tempid

Right

The follow on read works correctly - I end up with the right request going out, and data coming back

yeah, I get it...thinking...

But the on-load callback in data-fetch has an anonymous callback with a closure around the old temp id

I have a patch that I think might work by allowing the tempids to be rewritten in the standard way - if you want to take a look at what I'm thinking of as a fix

sure

So my thought is essentially you have something like this: https://github.com/AdStage/untangled-client/commit/68757aa83fc46fc75bb4a700e8abe23a163eca09#diff-5056e00e9ee472a1ae31c4dbb05b4685R55

Basically, allow extra arguments to pass through the network queue with :query, :on-load and :on-error. I haven't tested it, but I think it'd let the network plumbing pass in those arguments last minute, and give a handle to those hidden tempids that the rewrite-tempids function could take care of.

Given what I saw by tracing the requests, this should allow us to call load-data on something we only have a tempid for (assuming of course that the write that resolves the temp id definitely comes before the load-data).

I don't understand the internals enough to know if we could just use what we have already in query though, that might be another option

Since query is definitely getting rewritten successfully

So, let me summarize the problem: You're trying to do a single transaction that makes something, then loads a report on that thing from the server...but the thing you created has a temp id. My first comment is that it feel like bad design to begin with...if you just created it, what could the server possibly know about it that you don't? If it does, then what are you really creating (since the server already had data). This latter fact implies that you do know something related to the thing you're creating, and I'd suggest that this is the info that your follow-on read should be using.

Now, I do agree that Untangled does give a promise that if you queue something related to a tempid then it should "just work"

So, the former comment does not mean we don't have a bug that should be fixed

just noting that it is causing me some cognitive dissonance on the use-case

so, closing over a tempid is bad news

The server at the time of creation does not have the data-stream - it will take 3-4 seconds to fetch from our datasource and cache - so the hope is that rather than block the creation until an initial data-stream is available, we can do a follow on lazy-load for the data using the same lazy load we use elsewhere

OK, so on to your proposed solution...It seems sane. I'd have to review the code to remind myself...the nagging thing I'm not remembering is where the networking layer hooks in....it should be below this, so that plugging in another network implementation won't break your fixes.

And the only way to tack on that lazy load (without relying on the lifecycle methods to trigger it in componentDidMount - which I agree is worth avoiding), is to do it at the moment of widget creation.

Well, or to push a server side event that lets us know the data is ready to load in (our eventual goal)

That latter comment of yours is what I'm still pondering

yeah, the new websockets stuff would help...

so, your server implementation right now basically blocks on that particular request, which I'm guessing you're doing as a parallel request from the client

Yeah.

Until we get websockets worked in, that seemed like the cleanest way to handle the slow service we're wrapping.

Ah, and you want to use the real ident so it merges into the "right place" in app state

Right.

yeah, ok...I'm up to speed now

I completely agree this is a bug. I'm happy to take your patch as a PR once tested. The main criteria is that: - The change isn't visible to the API layer - The change doesn't break our ability to plug alternate networking I'm pretty sure what you've written meets those requirements.

Be glad to help you understand internals.

I could set aside some time to do a Skype consult with you guys if you'd like.

Sure thing - let me drop this in on our side and see if it actually works, then I'll see about cleaning it up.

great, thanks

Definitely need to figure out a better workflow for making changes to untangled from our app and testing them - but looks like this approach works.

Also, as far as I can tell with how this all works, I think it should maintain the API for other network implementations, since start-network-sequential-processing, where the main fix is applied takes any given network implementation.

One thought I had was maybe load callback and error callback should allow different arguments - I can see that maybe being necessary in some scenarios.

um, why?

https://github.com/untangled-web/untangled-client/pull/11/files#r63938600

^ I changed the timing of when the error callback's list of loading-items is generated, not sure if that matters at all, but if it did, we could use separate args to generate the list at the time it was generated before and pass only to the error callback.

Reviewing now....

Hm. I think those side-effects are important, but not sure why they got coded in that spot.

oh, no, those are not side-effects....just collecting up stuff from state

Bad name choices

the ! should not be on there...perhaps older versions were side-effecting

@ethangracer: originally wrote those, so perhaps he knows why the mutation naming was used

or did I 😕

either way, the names are poor

@therabidbanana: So, are you guys using checkouts while working with your app?

No, not currently

you're doing a localrepo deployment of snapshot?

Currently we just use the latest available jar. But to test this, I copied the src into our build.

ouch

you guys are using boot, aren't you?

Yeah

no checkouts support?

I'm sure we could figure something out, but nothing out of the box that I'm aware of

This maybe? https://github.com/EwenG/boot-checkouts

This also talks about the alternative in boot: https://www.martinklepsch.org/posts/why-boot-is-relevant-for-the-clojure-ecosystem.html

not sure it is right 😉

@therabidbanana: i was planning on setting that up for us after doing the modal stuff

Yeah, if you're patching U.C., it really makes a lot of sense

@tony.kay: pretty sure I named those in the early days before I understood what ! implied

agreed they shouldn’t be named that way

the changes in the PR don’t look like they’ll cause problems to me

@therabidbanana: If you fix that PR with my requests, I'll merge it and push a SNAPSHOT to clojars

Okay - something I didn't encounter because I did this on master - parallel reads.

let me know if you need a hand

The thing that's throwing me off is it looks like maybe develop is currently red?

I get an error even after stashing my changes

hm

possible, but undesirable 😉

Actual:	
▼Error: Assert failed: (reconciler? reconciler)
Error: Assert failed: (reconciler? reconciler) at om$next$app_state () at untangled$client$impl$data_fetch$mark_parallel_loading () at

let me take a look

But otherwise I think I applied the logic properly to parallel loads: https://github.com/untangled-web/untangled-client/pull/13

ok, I'll take a look at that while I'm at it

I've got all tests passing on develop

Maybe it's just my local machine - I maybe haven't called lein clean enough. 😄

entirely possible

I'm going to bump cljs clojure versions as well.

@therabidbanana: 0.4.9-SNAPSHOT in clojars

Hm. seems to break parallel loading

Odd - it should work the same, but I haven't tried enabling parallel loads anywhere yet. Let me try it out.

yeah, I'm running the cookbook background loading against it

yeah, your doseq is hosed

that is not a let 🙂

line 52

I'll fix it

Oh yeah, that's not going to work at all

Need to finish my afternoon coffee I guess

I guess I should be more picky about having PRs run the code they change before it gets merged 😉

Yeah, wasn't sure how best to test the new stuff since we haven't had a chance to add parallel yet.

the cookbook has a recipe

or just add :parallel true to any load data that doesn't care about order

fixed

deployed to clojars

@therabidbanana: did post-mutations break for you with your patch? Mine have stopped working...

i’m not seeing how your code would affect it, but nothing else has changed

I don't think they have - we only have one post-mutation in our code though.

Oh wait, maybe they have.

confirmed. loaded-callback is somehow being called with one thing as the items argument

since that one thing is a data state (which is map), the set-loading function is iterating over key value pairs instead of maps

can’t quite track down where that’s happening yet

but i’m observing it while debugging the loaded-callback in chrome, untangled.client.impl.data-fetch:268

I suspect it has something to do with action-with-args

I'm for some reason only seeing the post-mutations break down in with advanced optimizations

Or maybe just something about our development build triggers what we also have in a post-mutation so that it still works

I’ve reset my build and cleared cache a few times, pretty sure it’s not a compilation thing

not sure how the tests aren’t catching it though

yeah, rolled back a few commits and re-published to my local repo — post mutations work fine with the old, not with the new

i’ll keep digging, see if I can find it

I'm actually seeing that post-mutations work with the latest snapshot. Still trying to dig into it on my side.