Fork me on GitHub
#om
<
2016-09-15
>
jasonjckn00:09:11

@anmonteiro the parent of THIS is to one who aggregated THIS in it's render method?

jasonjckn00:09:51

it's weird I see the parent of a component as NULL but it's not at the root level

jasonjckn00:09:19

(only in optimized build)

jasonjckn00:09:56

going to try and upgrade everything to latest versions next

jasonjckn00:09:05

weird... I changed this in my code

jasonjckn00:09:19

-                (for [hit hits]
-                  (@SearchTableRow (om/computed hit {:columns columns
-                                                     [:search :tab] search-props})))))))))
+                (@SearchTableRow (om/computed (first hits) {:columns columns
+                                                            [:search :tab] search-props}))
+                )
and it fixed the NULL parent in simple optimized builds issue

jasonjckn00:09:39

for some reason that lazyseq is causing omcljs$parent not to be set correctly

jasonjckn00:09:55

flupot is aggregating it the lazyseq fyi

jasonjckn00:09:11

yup verified it's the lazyness that caused the issue

jasonjckn00:09:20

(tbody
                (doall
                 (for [hit hits]
                   (@SearchTableRow (om/computed hit {:columns columns
                                                      [:search :tab] search-props})))))

if you remove doall it breaks simple

jasonjckn01:09:40

the weirdest part out of everything is that tbody does a doseq on it's argument :S

mitchelkuijpers06:09:04

This is so freaking cool @anmonteiro! Server side rendering rocks

mping06:09:18

@anmonteiro love the work you’ve been doing, I think it really helps the community

molstt09:09:16

howdi! what's the status of om.next now? I'm reviving an old project using om 0.9.0 and om-bootstrap. do you recommend jumping to the alphas right away or stick to 0.9.0 if I want the smoothest sailing ahead? I will for sure go with om.next at some point, but not sure if it's the right time now..

petterik10:09:26

@molstt here's a clarification on om.now and om.next's relationship in the repo: https://github.com/omcljs/om/issues/765#issuecomment-246989800 Hopefully you find it helpful

molstt10:09:09

thanks, seems like I could use the alphas and gradually try out om.next which is nice!

petterik11:09:10

Components without om/IQuery can get passed nil next-props in reconcile!: https://github.com/omcljs/om/blob/master/src/main/om/next.cljc#L2531 If I'm understanding this code correctly, it's getting next-raw-props from (ui->props c) which parses the next props using the component's query and will always return nil for components without query. This makes sense, but what should we do about components without a query? Should those components not reach this code? Should components without a query never use props and always use om/computed instead? (if this is true, then we should warn usages of props without implementing om/IQuery)

petterik12:09:56

Actually, using om/computed doesn't work either, because (om/computed nil {:foo :bar}) => nil

petterik12:09:24

Overrided ui->props to return {} when not (om/iquery? c) enables me to use om/computed as a workaround for these components.

anmonteiro12:09:32

@petterik looks like a bug to me too

anmonteiro12:09:32

@petterik components without IQuery will only reach that code in case of a state update, I believe

anmonteiro12:09:57

so in that case I think we should just pass them the same props

anmonteiro12:09:35

if a parent component with IQuery updates those without will get passed the correct next props from the top anyways

anmonteiro12:09:07

I’m not too happy with these bugs in that code path

anmonteiro12:09:46

we should write some tests for that

petterik12:09:32

if a parent component *with* IQuery updates those without will get passed the correct next props from the top anyways, <-- exactly

jfntn13:09:57

Been struggling with remote tempids migration, I created a small repro case here: https://gist.github.com/julienfantin/26cacfda7fc9192a3ed5942534d934ca would love some feedback!

jfntn13:09:44

@anmonteiro thanks I did look at that and the test in fact fails

anmonteiro13:09:00

@jfntn just confirmed that gist works for me with alpha45

jfntn13:09:31

Thanks! Taking a look now

anmonteiro14:09:12

@petterik do you have a minimal repro for om-768?

anmonteiro14:09:06

I was trying to come up with a failing test but it seems to be passing because of https://github.com/omcljs/om/blob/master/src/main/om/next.cljc#L2536

anmonteiro15:09:01

@petterik or rather, could you try this branch locally and see if it fixes the bug? https://github.com/anmonteiro/om/tree/om-768

anmonteiro15:09:16

I think #768 is the result of an improper fix of #766 yesterday

jfntn16:09:34

@anmonteiro thanks a lot for the gist, I was able to find the problem after playing spot the differences for a while!

jfntn16:09:59

Main culprit was that in my send I was calling the 2-arity cb passing the remote as the second argument, but I’m not sure why that was wrong?

anmonteiro16:09:19

@jfntn probably the wrong query to normalize against the response

jfntn16:09:44

In this case it would have be the mutation e.g. '[(auth/register {:user/email …etc.. })]

anmonteiro16:09:14

@jfntn oh that’s the culprit then

anmonteiro16:09:31

normalizing against a mutation won’t work

anmonteiro16:09:53

you probably need to select the cases where you want to pass the 2nd argument to the callback

jfntn16:09:25

ok that makes sense

jfntn16:09:35

It was also surprising that the reconciler expects :tempids at the root but the remote parser will nest them under :result

mitchelkuijpers16:09:25

@jfntn: I noticed that normalization does not work with the 2 arity cb in the send function. When I removed it everything started working

anmonteiro16:09:44

@mitchelkuijpers that’s not exactly correct

jfntn16:09:53

@mitchelkuijpers is that the case for reads too?

anmonteiro16:09:08

the thing is: if you send a query as the 2nd arg, Om will use that to normalize the response data

mitchelkuijpers16:09:10

It did not work with reads (for me)

anmonteiro16:09:14

otherwise it’ll use the Root query

anmonteiro16:09:34

so the query you pass to the callback has to match the shape of the data

mitchelkuijpers16:09:38

It seemed to miss the metadata when I did that

mitchelkuijpers16:09:06

So it would not normalize.. But maybe I did something wrong

anmonteiro16:09:11

oh that would be a whole other issue

anmonteiro16:09:29

@mitchelkuijpers and probably a bug. Happy to look at a minimal case for that

mitchelkuijpers16:09:11

I'll try to make a minimal case this weekend @anmonteiro

jfntn16:09:36

@anmonteiro so would you recommend the 2-arity cb for remote reads?

anmonteiro16:09:53

@jfntn the answer is “it depends"

anmonteiro16:09:09

the contract is: the query must match the response data

anmonteiro16:09:21

as long as you satisfy that contract, everything should work

jfntn16:09:29

ok will keep that in mind

jfntn16:09:40

@anmonteiro thanks for your help!

peeja16:09:19

When figwheel reloads my app, I now have it .forceUpdateing my components, but that doesn't cause re-reads, so if I change a query, I have to refresh the page. Is there a way to tell a component to re-run its query definition and re-read?

peeja16:09:21

Oh, I guess the problem is really that reloading a defui (even with ^:once) doesn't update the instance's query, because the query is stateful. Is that right?

peeja16:09:42

Although it looks like get-query calls through to query unless the query has been set-query!'d, which mine hasn't…

petterik17:09:44

@anmonteiro I'm testing out your om-768 branch locally right now. FYI: I set-state! in my componentWillReceiveProps when the new props differ from the current props. It might be causing additional issues?

anmonteiro17:09:31

@petterik hope not. I think that branch should fix both #766 and #768

petterik17:09:23

It fixed #768 (!) but now I got #766

petterik17:09:31

Index out of bounds

anmonteiro17:09:28

@petterik can’t repro 766 with your minimal case

anmonteiro17:09:34

in that branch

anmonteiro17:09:05

well I did remove the guard that I added in the PR for #766

anmonteiro17:09:21

so the solution might be to just put it there again

petterik17:09:52

Yeah, I saw that the guard was gone. Putting the guard there will avoid the crash. But is it the right thing to do? Maybe?

anmonteiro17:09:13

@petterik probably asking too much, but could you put another repro together?

anmonteiro17:09:21

should maybe be easier than last time since you have yours to build upon now 🙂

petterik17:09:25

@anmonteiro not asking to much! I'll try

anmonteiro17:09:23

@cmcfarlen @peeja got any suggestions for the option name of the additional mutation? e.g. (compassus/set-route! app :home {:what-im-asking-you ‘[(set-route-params!)]}) https://github.com/compassus/compassus/issues/8

anmonteiro17:09:25

currently going for :tx

cmcfarlen17:09:37

Yeah, I was thinking route-txn. Is the optional param a map?

anmonteiro17:09:36

@cmcfarlen well, I suppose I was kinda predicting a change to set-route!, so I had already a 2nd param which is a map

anmonteiro17:09:57

currently only recognizes the :queue? key

anmonteiro17:09:03

so when #8 is solved, you’ll be able to (set-route! app :home {:tx ‘[(do/it! {:args ...})]})

anmonteiro17:09:38

doesn’t look ugly to me at all

cmcfarlen17:09:05

@anmonteiro: oh I see now. Looks good to me.

cmcfarlen17:09:49

@anmonteiro: I still owe you an example! Sorry, I forgot!

anmonteiro17:09:22

@cmcfarlen no problem, also not needed anymore I think?

cmcfarlen17:09:09

Perhaps not. It was going to show using bidi/pushy and setting initial route app state from the URL for deep linking to try to convince you to add something like #8. Ha

petterik19:09:53

@anmonteiro It looks like I've reproduced the Index out of bounds error again. Branch: https://github.com/petterik/om/tree/om-768-devcard the branch has your "Fix OM-766 and OM-768" commit

anmonteiro19:09:20

@petterik: awesome, thanks!

petterik19:09:52

It's not minimal actually. The second when form in componentWillReceiveProps is not needed. But yeah, I was about to give up, then I randomly found it 😅

anmonteiro19:09:00

@petterik ahh right seems like the problem is calling transact! on components that don’t implement IQuery

petterik19:09:15

Hopefully that's what's going on in my app as well. I don't think so actually, but we'll see 🙂

anmonteiro19:09:17

or rather, a combination of transact! and set-state! on such a component

petterik19:09:58

I'll keep trying to repro if it isn't the case

mitchelkuijpers19:09:34

@anmonteiro I was able to make a minimial reproduction of what I was running into if you call cb and give it the second parameter om/process-roots seems to be the problem but maybe you should not give that query to the cb but just the original.

anmonteiro19:09:47

@mitchelkuijpers oh right, you shouldn’t expect process-roots to give you a query that maintains metadata

mitchelkuijpers19:09:59

No that seems pretty logical

anmonteiro19:09:00

or maybe it needs fixing

mitchelkuijpers19:09:51

I have to be honest that I don’t really get the benefit you get from giving cb the query 👼

mitchelkuijpers19:09:05

Why would I want to have that working for me?

anmonteiro19:09:52

@mitchelkuijpers imagine you’re doing routing via union queries

anmonteiro19:09:13

and that the top-level key that aggregates the union doesn’t really exist in your app-state

anmonteiro19:09:40

you’ll never be able to get normalization to work with a response that doesn’t match that query

anmonteiro19:09:49

that’s only 1 example, I can easily think of more 🙂

mitchelkuijpers19:09:57

Aha that makes sense

mitchelkuijpers19:09:28

Oh thats pretty nifty actually! thnx @anmonteiro learned something new today

liamd19:09:15

where is om next at? is it stable enough to start a side project in?

petterik19:09:46

om.next is approaching beta. I'm not sure how much is going to change, but it's a lot of fun building products with. I recommend playing with it

anmonteiro20:09:45

@petterik well I think the solution will just be to put the guard back on

liamd20:09:36

cool, is the source for om/next on the master branch of the repo or is that om 1 or whatever?

petterik20:09:16

@anmonteiro I just pushed another repro which fails on doing update-state!

petterik20:09:33

Well, first transact! on one button, then clicking another button doing update-state!

petterik20:09:37

@anmonteiro Would it make sense to do this? https://gist.github.com/petterik/d03efa03bc73c216b93e316562d982a5 Props can only have changed with (ui->props c) if c has a query

anmonteiro20:09:45

@petterik no, we still need to be able to patch props up the subtree for components without IQuery

noonian20:09:04

@liamd om.next source is on master branch. Om.now is just in a different namespace.

liamd20:09:26

ah, got it

peeja20:09:00

When am I supposed to call process-roots? I thought it was supposed to be used in send, but it looks like the metadata has been stripped by then.

peeja20:09:23

Unless that's because of Compassus?

peeja20:09:44

@anmonteiro Do you know if Compassus might be breaking process-roots? Or am I just doing this wrong?

anmonteiro20:09:55

@petterik I think I have a fix

petterik20:09:58

@anmonteiro with the guard back on, my two devcards pass, but my app still gets nil props passed to componentWillReceiveProps

anmonteiro20:09:42

(same commit, force pushed because of the PR)

anmonteiro20:09:40

@peeja Compassus shouldn’t be stripping metadata

anmonteiro20:09:13

if it is I’m more than happy to fix it

peeja20:09:10

I must be missing something. I'm getting a different result from process-roots from my read and my send, and I'm not sure why. I thought maybe Compassus switching back and forth between queries and asts might be dropping the :query-root, but it should carry through correctly, right?

anmonteiro20:09:40

it should, can you produce a minimal failing case?

anmonteiro20:09:12

probably even a bug in Om if it’s happening in ast->query or query->ast

petterik20:09:09

@anmonteiro still isn't quite right. Getting next-props nil passed to shouldComponentUpdate, which leads to updating the component with nil props

anmonteiro21:09:13

@petterik doesn’t sound right to me

anmonteiro21:09:21

we only update the component’s props when they’re not nil

petterik21:09:16

Nope, it was just me 🙂

petterik21:09:19

everything seems to work!

anmonteiro21:09:39

awesome, glad to hear!

anmonteiro21:09:54

also glad I finally got it right. at least now we also have tests to prevent regressions

petterik21:09:56

we still get shouldComponentUpdate call with nil props, but that's fine?

anmonteiro21:09:13

that’s how it has been

anmonteiro21:09:35

I don’t know if we should maybe tweak that, I tried it locally and didn’t run into anything strange

anmonteiro21:09:47

but probably too much tweaking for one day 🙂

petterik21:09:02

So happy this got fixed

anmonteiro21:09:04

let’s maybe leave that for tomorrow 🙂

petterik21:09:34

I'm off to sleep now. Good night 🙂

anmonteiro21:09:54

thanks for the minimal cases! night

peeja21:09:34

@anmonteiro Hah, I was doing something dumb. I was comparing the query in read with the query in send, and of course the one in read is unwrapped by the first key, and that's why they're different. I think I'm confused, then: what's a :query-root, and who puts that metadata in the query?

anmonteiro21:09:59

@peeja you can put that metadata in the query

peeja21:09:18

Is it up to me, or is something supposed to be doing it automatically too, in certain cases?

anmonteiro21:09:21

either in your IQuery implementations or by associng :query-root to the AST

anmonteiro21:09:04

e.g.:

static om/IQuery
  (query [this]
    [‘[:current-user _] ^:query-root {:friends/list (om/get-query Friend)}])

anmonteiro21:09:24

that way process-roots will elide the link from that query

peeja21:09:25

Ah, interesting, I didn't realize that was appropriate

peeja21:09:22

Is there a way to turn [{:foo [:foo/bar {[thing/by-id] [:thing/name]}]}] into [{:foo [:foo/bar]} {[thing/by-id] [:thing/name]}] without manually adding ^:query-root? That is, always pull idents up to the top?

peeja21:09:31

I kind of expected that to happen automatically

anmonteiro21:09:24

no idea, I don’t really use process-roots

peeja21:09:33

Ah, well never mind then 🙂

anmonteiro21:09:55

probably something you can implement yourself

peeja21:09:23

Yeah, shouldn't be hard. I just worry when I do stuff like that that it means I'm going in the wrong direction 🙂