Fork me on GitHub
#untangled
<
2016-05-19
>
therabidbanana01:05:59

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

therabidbanana01:05:31

(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.

therabidbanana04:05:34

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

therabidbanana04:05:11

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

tony.kay17:05:24

@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.

therabidbanana17:05:26

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

therabidbanana17:05:39

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)

therabidbanana17:05:03

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

tony.kay17:05:09

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

therabidbanana17:05:36

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

tony.kay17:05:48

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

therabidbanana17:05:08

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

therabidbanana17:05:50

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

therabidbanana17:05:55

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.

therabidbanana17:05:14

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).

therabidbanana17:05:28

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

therabidbanana17:05:43

Since query is definitely getting rewritten successfully

tony.kay17:05:19

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.

tony.kay17:05:55

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

tony.kay17:05:08

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

tony.kay17:05:27

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

tony.kay17:05:51

so, closing over a tempid is bad news

therabidbanana17:05:52

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

tony.kay17:05:19

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.

therabidbanana17:05:06

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.

therabidbanana17:05:34

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

tony.kay17:05:35

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

tony.kay17:05:50

yeah, the new websockets stuff would help...

tony.kay17:05:43

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

therabidbanana17:05:30

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

tony.kay17:05:31

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

tony.kay17:05:51

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

tony.kay18:05:15

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.

tony.kay18:05:38

Be glad to help you understand internals.

tony.kay18:05:00

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

therabidbanana18:05:59

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

tony.kay18:05:46

great, thanks

therabidbanana18:05:29

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.

therabidbanana18:05:49

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.

therabidbanana19:05:05

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.

therabidbanana19:05:22

^ 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.

tony.kay19:05:49

Reviewing now....

tony.kay19:05:51

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

tony.kay19:05:49

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

tony.kay19:05:14

Bad name choices

tony.kay19:05:28

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

tony.kay19:05:52

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

tony.kay19:05:13

or did I 😕

tony.kay19:05:24

either way, the names are poor

tony.kay19:05:07

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

therabidbanana19:05:58

No, not currently

tony.kay19:05:17

you're doing a localrepo deployment of snapshot?

therabidbanana19:05:43

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

tony.kay19:05:26

you guys are using boot, aren't you?

tony.kay19:05:33

no checkouts support?

therabidbanana19:05:50

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

tony.kay19:05:03

not sure it is right 😉

currentoor19:05:03

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

tony.kay19:05:22

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

ethangracer19:05:02

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

ethangracer19:05:16

agreed they shouldn’t be named that way

ethangracer19:05:49

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

tony.kay20:05:37

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

therabidbanana20:05:36

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

tony.kay21:05:47

let me know if you need a hand

therabidbanana21:05:04

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

therabidbanana21:05:36

I get an error even after stashing my changes

tony.kay21:05:10

possible, but undesirable 😉

therabidbanana21:05:16

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

tony.kay21:05:31

let me take a look

therabidbanana21:05:27

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

tony.kay21:05:35

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

tony.kay21:05:59

I've got all tests passing on develop

therabidbanana21:05:39

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

tony.kay21:05:14

entirely possible

tony.kay21:05:06

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

tony.kay21:05:47

@therabidbanana: 0.4.9-SNAPSHOT in clojars

tony.kay21:05:05

Hm. seems to break parallel loading

therabidbanana22:05:56

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

tony.kay22:05:14

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

tony.kay22:05:16

yeah, your doseq is hosed

tony.kay22:05:23

that is not a let 🙂

tony.kay22:05:43

I'll fix it

therabidbanana22:05:11

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

therabidbanana22:05:47

Need to finish my afternoon coffee I guess

tony.kay22:05:58

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

therabidbanana22:05:02

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

tony.kay22:05:48

the cookbook has a recipe

tony.kay22:05:11

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

tony.kay22:05:57

deployed to clojars

ethangracer23:05:03

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

ethangracer23:05:23

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

therabidbanana23:05:45

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

therabidbanana23:05:16

Oh wait, maybe they have.

ethangracer23:05:35

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

ethangracer23:05:54

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

ethangracer23:05:01

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

ethangracer23:05:44

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

ethangracer23:05:34

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

therabidbanana23:05:05

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

therabidbanana23:05:29

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

ethangracer23:05:03

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

ethangracer23:05:09

not sure how the tests aren’t catching it though

ethangracer23:05:50

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

ethangracer23:05:10

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

therabidbanana23:05:48

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