Fork me on GitHub
#om
<
2016-09-01
>
levitanong09:09:31

@anmonteiro your advice was spot on! Thanks man

ethangracer13:09:48

@anmonteiro: at @tony.kay’s suggestion I tried replacing my recursive query with explicitly defined defuis. So I went from a query like [:db/id :tag/name {:tag/child ‘…}] to [:db/id :tag/name {:tag/child (om/get-query Tag2)}]. Limits the recursion but it’s exceptionally faster. Rerendering with the recursive query took 3.5 seconds. With the chained joins, it took 0.8 (with the exact same data set). Not sure what’s going algorithmically with path-meta and db->tree but seems like they could use optimization. Should I file an issue? Not sure what would be helpful in terms of documentation

selfsame13:09:53

@ethangracer huh, is this a sort of 'trampoline' likeTag1 Tag2 Tag1 etc.. or just recursive Tag2 joins?

ethangracer13:09:25

Tag1 -> Tag2 -> Tag3 -> Tag4 -> Tag5

ethangracer13:09:34

I just stopped after 5 levels

ethangracer13:09:58

@selfsame I could’ve been clearer about that above

ethangracer13:09:39

So even though my database supports infinite nesting, the UI would only unroll the first 5 levels with this implementation. But obviously huge performance benefits

selfsame14:09:04

@ethangracer and this is benchmarking the initial render?

ethangracer14:09:16

@selfsame correct, it’s not a re-render of the subcomponent but the first complete render of that view. Though subsequent rerenders are slow with recursive queries too

selfsame14:09:42

super interesting, guessing that recursive om/get-query isn't possible and this only works when you have a limit to node depth

anmonteiro14:09:35

@ethangracer 3.5 seconds definitely needs some kind of optimization 🙂

anmonteiro14:09:10

I’ve already spent some time optimizing path-meta in the past

anmonteiro14:09:00

so for me to go and look at it again I’d really appreciate a minimal case showing the slow path

ethangracer14:09:22

@selfsame I think it’s a call stack issue, I can’t image what else would cause that kind of performance difference in such a small data set

ethangracer14:09:13

@anmonteiro awesome, thanks — i’ll let you know

hlolli15:09:00

I notice that alpha42 has been on github for a month but alpha41 is the most recent version on clojars

peeja16:09:09

Based on https://anmonteiro.com/2016/01/writing-om-next-reloadable-code-a-checklist/, is there ever a reason not to (defui ^:once …) your components?

peeja16:09:27

Should that be the default behavior, rather than putting ^:once on everything?

grzm18:09:43

Looking at the property-based testing material (https://github.com/omcljs/om/wiki/Applying-Property-Based-Testing-to-User-Interfaces), would I want to remove the references to :remote in the mutate functions?

dnolen19:09:22

hrm I’m surprised no one has been bitten by om.next/transform-reads discarding parameterized reads

grzm19:09:27

That sounds ominous

dnolen19:09:42

but maybe people aren’t using parameterized reads that much yet

dnolen19:09:38

@anmonteiro have you run into this ^

anmonteiro19:09:58

@dnolen I don’t have for habit to use parameterized reads that much, at least not recently

anmonteiro19:09:05

where did this come up?

dnolen19:09:28

I just needed a parameterized remote read in a transact! and I saw it was getting lost

dnolen19:09:34

you wouldn’t see this with a normal query of course

anmonteiro19:09:52

could it be because of the indexer somehow?

anmonteiro19:09:00

I’m currently not looking at the code

dnolen19:09:01

no om.next/transform-reads is just naive

dnolen19:09:25

this just seems wrong to me

dnolen19:09:49

I guess the issue is that it assumes there’s a component that wants those properties

dnolen19:09:53

but if there isn’t, they go away

anmonteiro19:09:12

@dnolen well those keys don’t really belong to any component that the reconciler knows of

anmonteiro19:09:29

i.e. there’s no backing indexer

anmonteiro19:09:22

garbage in garbage out

dnolen19:09:32

yes but I’d still consider this bad behavior

dnolen19:09:39

if the reconciler can’t sort it out, it should leave them alone

dnolen19:09:50

the user intends something the reconciler doesn’t know about

anmonteiro19:09:02

it could just not touch those keys

dnolen19:09:21

(I’m just discussing this before I decide anything to make sure I’m not way off base here)

anmonteiro19:09:40

I only see one problem with leaving those untouched

anmonteiro19:09:03

which might not even be a problem at all

anmonteiro19:09:13

the user’s parser might not know how to handle those keys

anmonteiro19:09:51

but it’s really just a matter of adding support for that… so as I said, probably not even a problem

dnolen19:09:31

right so I’m not talking about that case

dnolen19:09:46

if you’re providing keys with no parser and no components - then something is definitely not right

dnolen19:09:55

but transform-reads happens before the parsing stuff

dnolen19:09:59

so keys just dissappear

dnolen19:09:09

if you hit parsing at least you would get “no multimethod for X"

dnolen19:09:34

but currently you get a transaction you didn’t write

dnolen19:09:39

and left scratching your head

anmonteiro19:09:49

I didn’t understand you were talking specifically about this scenario

anmonteiro19:09:23

right, at least I’d expect transform-reads to return those keys untouched

anmonteiro20:09:04

but I haven’t needed to transform-reads in that case

dnolen20:09:44

k I’m going to fix this, and then I think might just call the next release beta1

anmonteiro20:09:54

by curiosity, do you find that you need this use case?

anmonteiro20:09:14

or is it just something you want to make right?

dnolen20:09:15

@anmonteiro I think it’s just wrong and surprising, in my particuarly case someone wrote a super simple routing thing without bothering with union queries or anything like that

dnolen20:09:26

so the indexer never saw those keys

anmonteiro20:09:58

it’s definitely surprising, I can agree with that

dnolen20:09:00

I don’t want to mess with what they’ve done since it works just fine - except for this disappearing keys problem

dnolen20:09:57

we can always transition to union queries etc. later, but this should also work

anmonteiro20:09:05

no argument here

anmonteiro20:09:29

@dnolen btw just rebased the self-host PR

anmonteiro20:09:44

re: beta1, would be nice to include that one^

anmonteiro20:09:26

and probably the docstring one (I could put something together tomorrow)

dnolen20:09:55

@anmonteiro ok, I’ll cut an alpha today, and cut a beta after that one

anmonteiro20:09:10

that sounds good

dnolen20:09:20

@anmonteiro nice! I still to fix this other thing, might have to wait till tomorrow

anmonteiro20:09:47

@dnolen yeah, no rush for me

anmonteiro20:09:53

I’m still wondering about error handling

anmonteiro20:09:59

wasn’t that supposed to be up for beta?

dnolen20:09:09

@anmonteiro the design is mostly there already actually

anmonteiro20:09:14

also wondering about Routing support, is there anything else that you’re thinking about adding or do routing approaches just need to be documented?

anmonteiro20:09:21

(looking over the beta1 milestone)

dnolen20:09:24

I just didn’t do any super rigorous testing

dnolen20:09:32

I think what we have is fine for now

dnolen20:09:54

and if we come up something way, way better we can go with that

dnolen20:09:22

I’ve been building an app for a couple months now so I have a sense of what might need work, but the current set of features is workable and not a huge problem

anmonteiro20:09:41

was all that just re: error handling or routing too?

dnolen20:09:59

error handling and routing were the only two things I was really concerned about

dnolen20:09:12

your query propagation stuff fixed many of the big problems IMO

dnolen20:09:19

the error handling stuff just needs testing, finessing I think

anmonteiro20:09:35

also integration in the app lifecycle too I think?

anmonteiro20:09:46

is there a way to get at it currently?

dnolen20:09:36

maybe that’s the only part that’s missing, hooking it in

anmonteiro20:09:43

yea that what I thought

dnolen20:09:48

I’ll take look at that tomorrow too

anmonteiro20:09:43

@dnolen I have been thinking about something for a while now that I forgot to mention. It’s only supported in some newer browsers (so it would need polyfilling or at least checking if available), but I was thinking that we could hook up window.requestIdleCallback in some non-critical places

anmonteiro20:09:05

could potentially bring perf benefits

dnolen20:09:13

right, sure worth investigating!

anmonteiro20:09:24

Opening an issue to track that