This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2016-09-15
Channels
- # aws-lambda (3)
- # beginners (37)
- # boot (294)
- # carry (1)
- # cider (38)
- # cljs-dev (37)
- # cljsjs (88)
- # clojure (187)
- # clojure-android (2)
- # clojure-austin (1)
- # clojure-dusseldorf (9)
- # clojure-hk (3)
- # clojure-italy (12)
- # clojure-russia (36)
- # clojure-spec (55)
- # clojure-uk (27)
- # clojurescript (75)
- # community-development (5)
- # conf-proposals (2)
- # copenhagen-clojurians (3)
- # cursive (9)
- # datomic (54)
- # devcards (5)
- # devops (3)
- # dirac (69)
- # emacs (6)
- # ethereum (1)
- # euroclojure (1)
- # events (3)
- # funcool (1)
- # hoplon (20)
- # immutant (4)
- # luminus (14)
- # midje (4)
- # om (178)
- # om-next (2)
- # onyx (47)
- # pedestal (19)
- # protorepl (20)
- # re-frame (14)
- # reagent (54)
- # ring (2)
- # ring-swagger (7)
- # test-check (10)
- # uncomplicate (11)
- # untangled (9)
- # yada (9)
@anmonteiro the parent of THIS is to one who aggregated THIS in it's render method?
- (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(tbody
(doall
(for [hit hits]
(@SearchTableRow (om/computed hit {:columns columns
[:search :tab] search-props})))))
if you remove doall it breaks simpleThis is so freaking cool @anmonteiro! Server side rendering rocks
@anmonteiro love the work you’ve been doing, I think it really helps the community
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..
@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
thanks, seems like I could use the alphas and gradually try out om.next which is nice!
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
)
Actually, using om/computed
doesn't work either, because (om/computed nil {:foo :bar})
=> nil
Overrided ui->props
to return {}
when not (om/iquery? c)
enables me to use om/computed
as a workaround for these components.
@petterik looks like a bug to me too
@petterik components without IQuery
will only reach that code in case of a state update, I believe
so in that case I think we should just pass them the same props
if a parent component with IQuery
updates those without will get passed the correct next props from the top anyways
I’m not too happy with these bugs in that code path
we should write some tests for that
if a parent component *with* IQuery updates those without will get passed the correct next props from the top anyways
, <-- exactly
Been struggling with remote tempids migration, I created a small repro case here: https://gist.github.com/julienfantin/26cacfda7fc9192a3ed5942534d934ca would love some feedback!
@jfntn FWIW here’s one example you could look at: https://github.com/awkay/om-tutorial/blob/master/src/cards/om_tutorial/om_specs.cljs#L14
@anmonteiro thanks I did look at that and the test in fact fails
@jfntn oh and I’ve put a gist together some time ago: https://gist.github.com/anmonteiro/085d3d0636a3bc14f9f7
@jfntn just confirmed that gist works for me with alpha45
@petterik do you have a minimal repro for om-768?
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
@petterik or rather, could you try this branch locally and see if it fixes the bug? https://github.com/anmonteiro/om/tree/om-768
I think #768 is the result of an improper fix of #766 yesterday
@anmonteiro thanks a lot for the gist, I was able to find the problem after playing spot the differences for a while!
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?
@jfntn probably the wrong query to normalize against the response
In this case it would have be the mutation e.g. '[(auth/register {:user/email …etc.. })]
@jfntn oh that’s the culprit then
normalizing against a mutation won’t work
you probably need to select the cases where you want to pass the 2nd argument to the callback
It was also surprising that the reconciler expects :tempids
at the root but the remote parser will nest them under :result
@jfntn: I noticed that normalization does not work with the 2 arity cb in the send function. When I removed it everything started working
@mitchelkuijpers that’s not exactly correct
@mitchelkuijpers is that the case for reads too?
the thing is: if you send a query as the 2nd arg, Om will use that to normalize the response data
It did not work with reads (for me)
otherwise it’ll use the Root query
so the query you pass to the callback has to match the shape of the data
It seemed to miss the metadata when I did that
So it would not normalize.. But maybe I did something wrong
oh that would be a whole other issue
@mitchelkuijpers and probably a bug. Happy to look at a minimal case for that
I'll try to make a minimal case this weekend @anmonteiro
awesome
@anmonteiro so would you recommend the 2-arity cb for remote reads?
@jfntn the answer is “it depends"
the contract is: the query must match the response data
as long as you satisfy that contract, everything should work
@anmonteiro thanks for your help!
When figwheel reloads my app, I now have it .forceUpdate
ing 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?
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?
Although it looks like get-query
calls through to query
unless the query has been set-query!
'd, which mine hasn't…
@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?
@petterik hope not. I think that branch should fix both #766 and #768
noway
@petterik can’t repro 766 with your minimal case
in that branch
well I did remove the guard that I added in the PR for #766
so the solution might be to just put it there again
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?
@petterik probably asking too much, but could you put another repro together?
should maybe be easier than last time since you have yours to build upon now 🙂
@anmonteiro not asking to much! I'll try
@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
currently going for :tx
@cmcfarlen well, I suppose I was kinda predicting a change to set-route!
, so I had already a 2nd param which is a map
currently only recognizes the :queue?
key
so when #8 is solved, you’ll be able to (set-route! app :home {:tx ‘[(do/it! {:args ...})]})
doesn’t look ugly to me at all
@anmonteiro: oh I see now. Looks good to me.
@anmonteiro: I still owe you an example! Sorry, I forgot!
@cmcfarlen no problem, also not needed anymore I think?
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
@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
@petterik: awesome, thanks!
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 😅
@petterik ahh right seems like the problem is calling transact!
on components that don’t implement IQuery
Hopefully that's what's going on in my app as well. I don't think so actually, but we'll see 🙂
or rather, a combination of transact!
and set-state!
on such a component
@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.
@mitchelkuijpers oh right, you shouldn’t expect process-roots
to give you a query that maintains metadata
No that seems pretty logical
or maybe it needs fixing
I have to be honest that I don’t really get the benefit you get from giving cb the query 👼
Why would I want to have that working for me?
@mitchelkuijpers imagine you’re doing routing via union queries
and that the top-level key that aggregates the union doesn’t really exist in your app-state
you’ll never be able to get normalization to work with a response that doesn’t match that query
that’s only 1 example, I can easily think of more 🙂
Aha that makes sense
Oh thats pretty nifty actually! thnx @anmonteiro learned something new today
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
@petterik well I think the solution will just be to put the guard back on
cool, is the source for om/next on the master branch of the repo or is that om 1 or whatever?
@anmonteiro I just pushed another repro which fails on doing update-state!
Well, first transact!
on one button, then clicking another button doing update-state!
awesome
@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
@petterik no, we still need to be able to patch props up the subtree for components without IQuery
e.g.: https://github.com/omcljs/om/blob/master/src/devcards/om/devcards/tutorials.cljs#L137-L143
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.
@anmonteiro Do you know if Compassus might be breaking process-roots
? Or am I just doing this wrong?
@petterik I think I have a fix
@anmonteiro with the guard back on, my two devcards pass, but my app still gets nil
props passed to componentWillReceiveProps
(same commit, force pushed because of the PR)
@anmonteiro Testing it now
@peeja Compassus shouldn’t be stripping metadata
if it is I’m more than happy to fix it
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?
it should, can you produce a minimal failing case?
probably even a bug in Om if it’s happening in ast->query
or query->ast
@anmonteiro still isn't quite right. Getting next-props nil
passed to shouldComponentUpdate
, which leads to updating the component with nil
props
@petterik doesn’t sound right to me
we only update the component’s props when they’re not nil
awesome, glad to hear!
also glad I finally got it right. at least now we also have tests to prevent regressions
that’s how it has been
I don’t know if we should maybe tweak that, I tried it locally and didn’t run into anything strange
but probably too much tweaking for one day 🙂
let’s maybe leave that for tomorrow 🙂
thanks for the minimal cases! night
@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?
@peeja you can put that metadata in the query
Is it up to me, or is something supposed to be doing it automatically too, in certain cases?
either in your IQuery
implementations or by associng :query-root
to the AST
e.g.:
static om/IQuery
(query [this]
[‘[:current-user _] ^:query-root {:friends/list (om/get-query Friend)}])
that way process-roots
will elide the link from that query
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?
no idea, I don’t really use process-roots
probably something you can implement yourself
Yeah, shouldn't be hard. I just worry when I do stuff like that that it means I'm going in the wrong direction 🙂
right