Fork me on GitHub
#om
<
2016-02-17
>
hueyp00:02:24

a second question might be what is the right way to transact! unique-idents … do you transact! :current-user or [:current-user _]. this is sorta big for us since if its :current-user that gets filtered out by transform-reads if its not mounted … I’m not sure how to transact! unique-Idents in a way that gets both sent to a remote always and updates the ui 😛

hueyp00:02:03

I’m afraid the second issue might be a design decision … that you can’t transact things that aren’t mounted

drcode03:02:45

Hi, is it normal form a "read" function not to know what parts of the inner query are remote vs. local and then use the parser to query the inner joins to determine this? For instance, I have the following code:

defmethod read :retail
           [{:keys [state parser query ast] :as env} key _]
           (let [env (assoc env :retail (@state key))
                 remote-part (parser env query :remote)]
                {:value  (parser env query)
                 :remote (when (seq remote-part)
                               (assoc ast :query remote-part))}))
As you can see, it figures out the "remote-part" via the inner query- This code seems to work but also seems a bit ugly to me... can someone give me an opinion or suggest a way that I can do this in a cleaner way? Thanks!

hueyp03:02:13

no idea of idiomatic, but awkay had the same idea

drcode03:02:27

@hueyp: Thanks, very interesting alternative take on the same idea!

soxley04:02:49

can someone tell me why this doesn't work?

cljs.user=> (def state (atom {:foo {:bar true}}))
#'cljs.user/state
cljs.user=> (def state-cursor (om/root-cursor state))
#'cljs.user/state-cursor
cljs.user=> (om/update! state-cursor [:foo :bar] false)
#object[Error Error: No protocol method INotify.-notify! defined for type cljs.core/Atom: [object Object]]

Error: No protocol method INotify.-notify! defined for type cljs.core/Atom: [object Object]
    at cljs$core$missing_protocol ()
    at om$core$_notify_BANG_ ()
    at om$core$notify_STAR_ ()
    at om$core$transact_STAR_ ()
    at $core$ITransact$_transact_BANG_$arity$4 ()
    at om$core$_transact_BANG_ ()
    at Function.om.core.transact_BANG_.cljs$core$IFn$_invoke$arity$4 ()
    at om$core$transact_BANG_ ()
    at Function.om.core.update_BANG_.cljs$core$IFn$_invoke$arity$3 ()
    at om$core$update_BANG_ ()

soxley04:02:14

I could have sworn I tested this before and it did work.

soxley04:02:20

I think I figured it out. I needed to call om/ref-cursor on the cursor

soxley04:02:41

cljs.user=> (def state (atom {:foo {:bar true}}))
#'cljs.user/state
cljs.user=> (def state-cursor (om/root-cursor state))
#'cljs.user/state-cursor
cljs.user=> (om/update! (om/ref-cursor state-cursor) [:foo :bar] false)
nil
cljs.user=> state
#object [cljs.core.Atom {:val {:foo {:bar false}}}]

anmonteiro13:02:54

@dnolen: hoping you could clarify something for me regarding the index building. I think I'm seeing a bug. in the 2 queries below, are the prop->classes index keys correct?

[:foo {[:current-user _] [:name]}]
;;=>  #{:foo [:current-user _] :name}       ->  (shouldn't it be :current-user without the link?)

[:foo {[:users/by-id 2] [:name]}]
;;=> #{:foo [:users/by-id 2] :name}       ->  (shouldn't it be :users/by-id without the indent?)

dnolen13:02:36

@anmonteiro: for the later it should be the ident

dnolen13:02:54

we don’t want to look every component with that ident key

anmonteiro13:02:23

@dnolen: I'm seeing 2 bugs then, I believe:

[[:users/by-id 2]]
;; #{:users/by-id} ;; Incorrect, should be the ident

[{[:users/by-id 2] [:id :name :age]}]
;; #{[:users/by-id 2] :id :name :age} ;; Correct

[{[:current-user _]} [:name]]
;; #{[:current-user _] :name} ;; Incorrect, should be #{:current-user :name}

[[:current-user _]]
;; #{:current-user} ;; Correct

anmonteiro13:02:59

Filing issue once you confirm this

dnolen13:02:05

@anmonteiro: sorry not following

dnolen13:02:14

are those two supposed to be bugs? I find the comments here confusing

anmonteiro13:02:44

let me try to clarify in the snippet

dnolen14:02:23

@anmonteiro: ok, yes those are how it should be

anmonteiro14:02:15

thx for the clarification, filing issue

anmonteiro14:02:42

@dnolen: I've also been thinking if we could add an invariant that checks if components are stealing queries from each other

anmonteiro14:02:55

by looking at the indexes, I believe it should be possible

dnolen14:02:19

@anmonteiro: that would be very useful

anmonteiro14:02:38

I haven't put any time into actually proving we can do it, but I'll try to play around with it later today

dnolen14:02:59

I have a simple check but something stronger would be interesting (with the caveat that it doesn’t introduce more problems)

anmonteiro14:02:17

Yes, you do have a simple check in get-query, I believe

dnolen14:02:19

edge case to watch out for … you may want to use the same query with N components

anmonteiro14:02:42

but this is something that is only verified after the fact. If it were in build-index*, feedback would be immediate

dnolen14:02:44

so the query value needs to unique - perhaps full query is enough?

dnolen14:02:52

but you have to watch for unions I think

dnolen14:02:09

(but maybe this was sorted when I dealt with unions, I don’t remember)

anmonteiro14:02:24

I can't confirm any of that from the top of my head without actually digging in

anmonteiro14:02:06

I'll make sure to watch out for those cases you mention nevertheless

jlongster14:02:55

after some more detailed profiling I discovered that db->tree was only 10ms out of a 25ms query. I added more detail timing information and discovered that path->meta is taking a lot of that time. It's called on the returned value, and because this is a top-level tabs component it's returning a big data structure when switching, which path->meta is slow on. Looking at the source, I discovered the :elide-paths option which fixes the issue, but what are the consequences of using that option?

dnolen14:02:29

@jlongster: it’s an expedient at the moment, there’s an existing issue which will be the real fix

dnolen14:02:40

it’s not that hard I might take a look at it this week

jlongster14:02:07

@dnolen: cool, link to issue? any chance I might be able to work on it?

dnolen14:02:43

@jlongster: yes you could if you like, it’s not particularly hard

dnolen14:02:08

currently we blindly recurse instead of using the given query to figure out which parts of the data actually need to be traversed

jlongster14:02:21

I'm not entirely sure what the path metadata is used for, so I may not be able to fix it quickly. Not sure exactly what you mean, so I can just wait for a fix simple_smile

jlongster14:02:12

another question: why do we need to re-run the queries when calling set-state!?

marianoguerra14:02:32

one question, I was getting some weird errors when om was handling a response from a remote, I fixed it by returning the response as a vector instead of a list, my question is, does the list/vectore difference have a reason for the response? if so, is there a way to warn when the user returns the response as a list?

marianoguerra14:02:22

when I say list I mean {:value (...)} vs {:value [...]}

dnolen14:02:34

@jlongster: path metadata - it’s for the cases where we can’t for some reason use :pathopt - the path metadata lets run only part of the query from the root and then just get what we need

dnolen14:02:51

@marianoguerra: it’s possible but the answer is more subtle than that

dnolen14:02:03

maps and vectors are only required wrt. query response structure

dnolen14:02:07

it’s not like lists aren’t allowed

dnolen14:02:48

so a warning would need to look at the query to sort this out - so the right time to do this is durning normalization

dnolen14:02:00

ticket + patch welcome

marianoguerra14:02:11

@dnolen: ok, I'm finishing an example with frontend and backend and I will also document the problems I had and causes so people don't have the same simple_smile

dnolen14:02:26

@jlongster: re: set-state! I can’t remember at the moment why I did it this way - but I recall there was some reason but I don’t have time to load it up right now

dnolen14:02:01

it may also be an oversight, but I did spend some time on it - looking at the source would probably clear things up

jlongster14:02:32

@dnolen: ok that's cool. It would neat if set-state! could be used for optimization purposes like in this case

dnolen14:02:40

async rendering loop - there’s no evidence that the set-state! call is by itself

dnolen14:02:46

we don’t immediately invoke forceUpdate ever

jlongster15:02:04

true, but maybe there is a way to track that?

dnolen15:02:33

@jlongster: probably - would be a useful perf tweak

jlongster15:02:51

ok cool, maybe I'll take a crack at that if you think it's sane

dnolen15:02:22

it seems sensible to me - you would just need to keep track of components who only get set-state! call and no transact!

dnolen15:02:26

easy to do this in the reconciler

dnolen15:02:38

(the tracking)

dnolen15:02:52

and then in the render loop in the reconciler just skip recomputing queries for these components

jlongster15:02:49

awesome, I'll try it out!

grzm15:02:34

om.next includes the following functions: get-query, get-unbound-query, get-params. Am I understanding correctly that get-query works on component classes and instances, while the latter two only work on instances? I'm seeing the error Assert failed: (component? c) when calling get-unbound-query and get-params on component classes.

grzm15:02:56

I understand the docs are in incomplete and in flux, but it seems that they're not always consistent in distinguishing between component classes and component instances

grzm15:02:55

Answering my own question: yes, get-unbound-query and get-params work on component instances only, not component classes.

jlongster15:02:47

that makes sense to me

jlongster15:02:51

I need to figure out how to run Om's tests, is there a page for that?

anmonteiro15:02:26

@jlongster: (require 'om.next.tests) (in-ns 'om.next.tests) (run-tests)

jlongster15:02:14

@anmonteiro: I get a "no such namespace" error. do I need to install om any differently?

anmonteiro15:02:03

@jlongster: wait, how are you running them?

anmonteiro15:02:20

you can either run them at the Node REPL or the figwheel REPL

anmonteiro15:02:27

inside the Om project

anmonteiro15:02:30

without installing

jlongster15:02:42

oh, I'm in my project

jlongster15:02:47

I see thanks

jlongster15:02:58

@anmonteiro: how do you start a figwheel REPL within Om? Sorry, I'm pretty new to Clojure's tooling

anmonteiro15:02:07

it's alright

anmonteiro15:02:27

rlwrap lein trampoline run -m clojure.main script/figwheel.clj

anmonteiro15:02:54

rlwrap is not necessary

anmonteiro15:02:01

just for completion

anmonteiro15:02:15

I like to use it for cmd line history

jlongster15:02:27

makes sense, thanks!

grzm15:02:53

@anmonteiro: what's the advantage of using lein trampoline run as opposed to just lein run?

anmonteiro16:02:01

trampoline spawns another process and kills lein

jlongster16:02:03

@anmonteiro: argh, sorry, so now figwheel is waiting for something to connect, what do I run in node to create a REPL environment?

anmonteiro16:02:17

lein run doesn't kill lein

anmonteiro16:02:40

@jlongster: oh right, you need to point your browser to

anmonteiro16:02:59

to run the node REPL: rlwrap lein trampoline run -m clojure.main script/repl.clj

akiva16:02:29

Actually, my understanding is that without trampoline, Leiningen spawns two processes; one for itself and one for what it’s running.

akiva16:02:47

trampoline launches the command and ends itself.

jlongster16:02:50

@anmonteiro: aah cool, works now thanks

anmonteiro16:02:46

@akiva: yes, you're right

akiva16:02:32

If lein trampoline ever fails out on you, just run a lein clean first.

dnolen16:02:09

@jlongster: cool will take a look later

dnolen16:02:23

@jlongster: to merge that will need a CA

jlongster16:02:48

@dnolen: ok I have to request one from you, right? or is it online somewhere

dnolen16:02:26

@jlongster: I have your email, I’ll send you one now

hueyp17:02:46

@anmonteiro: not sure if at all related to the stuff you are looking at in the indexer, but that repro I sent you, if you notice the transformed reads end up being : [(user/update) {:thing-a []} {:thing-b []}]

hueyp17:02:12

so that works since it re-renders the components, but the actual field that changed is left out of the query which would screw with remote integration

hueyp17:02:34

i.e. :current-user

hueyp17:02:59

I didn’t notice that until later since I was just looking at the local ui

anmonteiro18:02:30

@hueyp: I've submitted a patch that fixes it

hueyp18:02:22

do you know if that manifests itself on the transform-reads stuff? I can try your PR out locally too~

anmonteiro18:02:11

the transform-reads fn uses the indexes and they were getting built incorrectly

anmonteiro18:02:19

wrt to idents & links

hueyp18:02:55

nice, I’ll have to try it out!

grzm18:02:50

just fired up the om-next todomvc project (with @anmonteiro 's pull request https://github.com/swannodette/om-next-demo/pull/2 ). Can't seem to add a new todo item (tried Safari and Chrome). Before I dig too much into it, anyone else experience this issue?

george.w.singer18:02:02

From the om.next docs: "render should return an Om Next or React component." Isn't that wrong? Shouldn't it return a React.Element (i.e., something like (om.dom/div nil "text") )?

george.w.singer18:02:00

According to my understanding: A component is a function that takes in props (among other things) and returns a React.Element. A React.Element is not a component.

jlongster19:02:48

@george.w.singer: very common to call those components, and refer to the type as a "component class"

george.w.singer19:02:50

@jlongster: Got it. Just making sure I wasn't missing something more fundamental simple_smile

iwankaramazow19:02:22

is there any heuristic about the length your queries should take (time) when I should start worrying?

iwankaramazow19:02:53

i.e. some query takes 30msecs atm

iwankaramazow19:02:26

Om flags a warning, optimize now?

dnolen20:02:27

@iwankaramazow: it’s just a way to hint that you should start considering :pathopt and memoization where appropriate

dnolen20:02:56

30ms isn’t terrible but you’re dropping frames

anmonteiro20:02:15

@dnolen: there's something about updating queries that's been bugging me. Let's say we have components Parent & Child. As expected, Parent uses (get-query Child) If we set the child's query to a different one, reindexing will occur. What I'm seeing is that the indexes are not updated because when running the indexing from the root instance, get-query won't see anything in -> state ::queries for the root and will fallback on getting the class query, which in turn gets the query from Child's class.

dnolen20:02:40

@anmonteiro: right this is a known problem

dnolen20:02:53

this is the idea behind subquery

dnolen20:02:17

but that’s a half-baked idea

dnolen20:02:41

or possibly a half-baked idea

dnolen20:02:42

I don’t know

anmonteiro20:02:44

this might have other implications downstream

dnolen20:02:12

the other approach is to have a query manager like thing, but I haven't had the time to work through whether that’s a good idea

anmonteiro20:02:13

though it hasn't been a problem for me yet, it seems like there could be distinct views on the queries of an app

dnolen20:02:44

and if it is a good idea - whether it’s just good enough to trump the half-baked ones

anmonteiro20:02:47

depending where you look from

dnolen20:02:06

yes - this is the real problem

dnolen20:02:14

the interest in “routing” is not the real problem

dnolen20:02:59

I’m also happy to see somebody spend some brain cycles on it if they feel inclined

dnolen20:02:19

playing around with some query manager like thing

anmonteiro20:02:28

I'm happy to think about the problem, and bug you with questions down the road

anmonteiro20:02:06

the first one would be: is there an interest in keeping the original query?

dnolen20:02:34

@anmonteiro: I just don’t see how it can be all that useful, you can always get back to it via the class anyway

dnolen20:02:09

the co-locating query thing is in fact a feature which is mostly nice for humans

dnolen20:02:27

a query manager could for example manage all the details

dnolen20:02:04

(defui Foo static IQuery (query [this] (get-query manager this)))

dnolen20:02:22

maybe the query manager could go into the reconciler a la indexer

anmonteiro20:02:25

The most obvious pain point with the current approach is that full-query returns wrong results at times

anmonteiro20:02:29

because it relies on the indexes

dnolen20:02:31

lots of questions here

anmonteiro20:02:51

it seems to me that the indexes should be updated at all times

anmonteiro20:02:25

I'm thinking that if the query manager presents as a viable solution, queries could still be collocated and we'd manage the dirty manager stuff behind the scenes in defui

anmonteiro20:02:30

i.e. don't touch the current public facing API, but make it behave differently inside

jlongster20:02:32

I was curious about Relay, it doesn't look like it has an equivalent set-query! does it? Surely there's some way to change the query, but I don't see it https://facebook.github.io/relay/docs/api-reference-relay-container.html

anmonteiro20:02:45

@jlongster: that link mentions setVariables

anmonteiro21:02:07

isn't that it?

jlongster21:02:28

@anmonteiro: I think that's just the equivalent to changing query params, not the query itself

hueyp21:02:56

you handle set-query! query part in Relay at a root container, and then can set query params on any component

arohner21:02:14

TIL you can’t call om/update! on an app-state until after om/root is called

hueyp21:02:33

I think it might be analogous to routing via set-query! at a root in om

hueyp21:02:51

the difference being the relay root container can swap both component + query at once

hueyp21:02:49

in om.next you might unmount / remount a new root or have a kind of indirection root component that does a switch on what to render … that is vaguely what the root container is doing

hueyp21:02:28

the other take in Relay is directives … you can make parts of a query conditional which get ignored via variables … include(if: $selected) … which is something you might use update-query! or set-query! to do

jlongster21:02:07

oh that's neat, yeah that's a basic use case

hueyp21:02:41

I think ideally, like Facebook, has a single root per route … but using react-router-relay that didn’t match (router passes you children) so you end up with a relay root container per route ‘level'

anmonteiro21:02:27

@seanirby: answering your question, the ident seems correct but you need to compose the queries to the root

anmonteiro21:02:07

this means that the PeopleWidget query needs to get-query the Person's query

seanirby21:02:43

anmonteiro: ah ok, I knew I was passing the raw data, so I need to pass a query instead. makes sense

anmonteiro21:02:24

you need to do both

anmonteiro21:02:46

query for the children's queries

anmonteiro21:02:51

and pass their props accordingly

seanirby22:02:08

anmonteiro: I don't follow on the last bit. Does that mean I need to use db->tree ?

anmonteiro22:02:35

I'm only talking about the defui implementation

anmonteiro22:02:52

db->tree is used to denormalize the data, but commonly used in your parser

seanirby22:02:22

anmonteiro: I get it now, you get the query for the paren'ts children in the parent's query.

seanirby22:02:46

anmonteiro: thanks for the help simple_smile

adamkowalski23:02:09

What is the idiomatic way in Om Next to deal with event streams. For example in Elm you would have a notion of a signal which could represent a time varying value such as a mouse position, the window dimensions, what keys are being pressed, etc.

adamkowalski23:02:34

Should I essentially set global event listeners for the events that I care about, and then transact the latest information to the global app state atom?

adamkowalski23:02:17

for example every time the mouse is moved I would fire an event which would cause a transaction to occur which has a payload containing a vector with the current mouse coordinates?

anmonteiro23:02:47

@adamkowalski: It is my opinion that if you start firing different events according to what you're trying to do (mouse position, window dimensions, several keys) you'll end up losing control rather quickly

tony.kay23:02:02

Hot code reload seems to have started to be less useful due to recent change to shouldComponentUpdate....forceRootRender doesn't re-render anything because S.C.U. returns false (since app state has not changed)

anmonteiro23:02:26

my approach would be to listen to those events and put all of them in the same core.async channel

anmonteiro23:02:56

and have one handler for all the events that dispatches on whatever you define

tony.kay23:02:58

@anmonteiro: Seems to have stopped working with your change on 2/4

tony.kay23:02:44

We should probably add some kind of marker that shouldComponentUpdate can see in order to force all components to rerender when we ask for a re-render, no?