Fork me on GitHub
#fulcro
<
2017-12-01
>
cjmurphy02:12:34

Just noticed that transact! has another arity that has ref as a second arg: [r ref tx] (usually it is [r tx]). Is ref where you pass in an ident? What's the purpose of ref? I just discovered this while reading the code of fulcro-inspect.

wilkerlucio10:12:38

@cjmurphy when do it the ref gets in your mutation as part of the environment, it's a good practice to use the ref to target the element on your mutations, this works as mechanism that you can use to trigger actions to any entity based on ref on your system

wilkerlucio10:12:03

for example, check the default mutations of Fulcro, like set-props!, it uses the ref to target the data on the db

tony.kay15:12:01

@currentoor never seen it…REPL screw up?

tony.kay15:12:15

I’ve definitely seen cases where I didn’t have what I thought I had

tony.kay16:12:28

@grzm I don’t personally use CIDER, and have very little experience with it. The instructions are contributed. If they are in error, I’d appreciate help 🙂

tony.kay16:12:49

It is almost certainly an oversight

cjmurphy16:12:39

Thanks @wilkerlucio. I can see on the definition side of defmutations that there's a ref in the env. In practical terms I just see using ref as easier than going to the effort of passing a parameter.

tony.kay16:12:02

@cjmurphy I would not encourage that unless you’re tying a mutation to a component tightly

tony.kay16:12:20

if the mutation gets executed from an arbitrary context, ref will be the ident of the component that invoked it

tony.kay16:12:24

it could make life miserable…basically that arg is really used when you’re running transact! on the reconciler, but you’re using a mutation that relies on ref. Personally, I never code it that way because I feel parameters are much clearer, well-documented, code assist, etc.

cjmurphy16:12:57

Right - only use for mutations that only make sense when they are related to a component instance (an ident). Seems not that necessary for normal application programming - only needed for reading other people's code.

tony.kay16:12:15

One thing, though: I think if you include a ref to transact it does queue that ref for re-rendering. Of course, you can put that ident in the follow-on reads and get the same thing.

tony.kay16:12:15

the ref in env is either derived from the component running the tx, or one you explicitly state. That ident is queued for refresh.

tony.kay16:12:25

That’s all. Use it however you feel is “ok” 🙂

tony.kay16:12:09

I personally think it a questionable addition to the API

cjmurphy16:12:19

That makes sense thanks. Esp good to know it will queue for re-rendering.

tony.kay16:12:36

well, you can queue re-render with the ident in the follow-on reads

tony.kay16:12:17

and using it to establish context is always going to be inferior to sending it in as a param to the mutation that needs it…because it causes coupling. That’s a theoretical opinion, and in practice it can be convenient

cjmurphy16:12:35

Yes but when reading code good to know that its equiv to follow on read.

tony.kay16:12:40

(transact! this ident [(f) (g) (h)]) is less typing than sending each a param

tony.kay16:12:14

(transact! this [(f {:i ident}) (g {:i ident}) (h {:i ident})])

tony.kay16:12:55

but, how often would this latter case come up? In practice, you’d probably only need it in one of those…so, again, don’t personally like it.

wilkerlucio17:12:10

I really like the transact with ref, in fulcro-inspect this feature is used to target components that are deep into the ui tree, without having to know exactly how to get there, I set some id's manually for then, so then I can just use their ref to apply changes to then, without having to care where they are

tony.kay17:12:22

oh, I see…so you’re using a mutation you wrote, that targets components you didn’t write?

tony.kay17:12:07

but you want to write the mutation so it can be told which one…still, you could just be saying:

(transact this `[(do-thing {:to ident}) ident])

tony.kay17:12:24

but I agree for that use-case it is more wordy

tony.kay17:12:09

I guess I can see that making mutations for a “kind of thing”, which is common, would benefit from this when you have to target them from “outside” (e.g. server push)

tony.kay17:12:54

(transact! reconciler [:person/by-id 1] `[(person/increment-age)])

tony.kay17:12:20

instead of

(transact! reconciler `[(person/increment-age {:id 1})])

wilkerlucio17:12:29

yeah, and with the defmutation it just looks like calling functions, just in a different "env space"

wilkerlucio17:12:51

if the change includes server changes, then I usually add the params as well

tony.kay17:12:54

personally I still prefer the latter…it is more clear to me…it does not rely on context + params

wilkerlucio17:12:07

I see the ref case been more common in client-only components

tony.kay17:12:08

and like you said : it works with the server

wilkerlucio17:12:33

or better saying, more on client-only mutations in general

tony.kay17:12:49

Yeah, I just don’t see any goodness from it 😉 It seems like it just makes a function that is context sensitive, which leads to subtle bugs

wilkerlucio17:12:03

things like: open that tab, select that modal, add this entry (local only)

tony.kay17:12:11

still dislike it 😄

wilkerlucio17:12:26

well, just please keep it there 🙂

wilkerlucio17:12:18

I've having a performance problem a bit, I would like to know if you have an idea about how I could improve it

wilkerlucio17:12:32

it's a very unusual case

wilkerlucio17:12:38

I'm porting a big reframe app to fulcro

wilkerlucio17:12:42

but we are doing it in steps

wilkerlucio17:12:49

I have a screen filled with many many widgets

wilkerlucio17:12:56

most of then in reframe at this point

wilkerlucio17:12:05

we are adding a second fulcro widget there

wilkerlucio17:12:25

so, in order to leverage a single query (which is the main purpose, have a single fulcro app there)

wilkerlucio17:12:39

what I'm doing is a fulcro app, that contains a mixed of fulcro and reframe components inside of it

wilkerlucio17:12:54

so we can swipe one by one of the widgets

tony.kay17:12:08

ok…so you have stateless components and statefull stuff…got it

wilkerlucio17:12:14

but when I did this wrap, the fulcro components are getting slow to update =/

wilkerlucio17:12:38

when I click on the things, I can feel the slow (I guess around 500ms), even when it's just a boolean change on the app state

tony.kay17:12:58

OK…so. how many of the components have queries, overall, in the fulcro part?

wilkerlucio17:12:16

all of then, which are 2 at this point

tony.kay17:12:26

ok, so it isn’t query performance 🙂

wilkerlucio17:12:39

if I just make the fulcro part, they go fast

wilkerlucio17:12:47

the problem seems to be around the mixed rendering

tony.kay17:12:49

Did you end up with two React versions at once?

tony.kay17:12:11

the reframe stuff…how does it get data?

wilkerlucio17:12:42

with those subscriptions and all that signal stuff he does

tony.kay17:12:16

have you looked at the flame map with chrome?

tony.kay17:12:00

yeah, that should make it more obvious. My guess is that something is getting triggered into looping…If you’re embedding reframe stuff in fulcro components, you’ll need to isolate them so the targeted refresh of fulcro doesn’t cause them to refresh.

tony.kay17:12:07

(guessing here)

tony.kay17:12:56

any forceUpdate on a component (which is what Om Next does) will re-render the subtree (normally the Om factories prevent real rendering of children)…but reframe might treat that as the “app starting” again or something, because it might think it “owns” the whole tree

wilkerlucio17:12:24

yeah, I'm guessing it's trying to update the whole thing on events

wilkerlucio17:12:50

when a fulcro component updates (via (mutations/set-value!) for eg), the rendering is localized to it?

tony.kay17:12:23

so, depends on 1.x vs. 2.x

wilkerlucio17:12:38

how is it in 1.x?

tony.kay18:12:18

In 1.x it is Om Next: set-value! will update the local component’s props, and trigger a refresh for that component, BUT without path-opt, it has to render from the root.

tony.kay18:12:36

shouldComponentUpdate is what short-circuits things that have not changed

tony.kay18:12:06

Om Next optimizes how much of the query it must run…but rendering happens from root without path-opt

tony.kay18:12:20

oh wait…that’s not true

wilkerlucio18:12:20

humm, that sounds like an issue for the case I'm handling here

tony.kay18:12:42

the query runs from root…the update can still be component local

tony.kay18:12:47

sorry…my bad on that

tony.kay18:12:54

can is the operative term

tony.kay18:12:03

not necessarily will

tony.kay18:12:38

depends on follow-on reads…but in the case you described (`set-value!`), it should be local to that component….throw in some console logging 🙂

wilkerlucio18:12:57

yeah, I have one in the main part, that seems to not be triggering

wilkerlucio18:12:07

I have to add more into the reframe components to see if something is going on there

tony.kay18:12:10

Om always update the this subtree of the transact!

wilkerlucio18:12:23

ok, this tree is fine 🙂

wilkerlucio18:12:52

but if that was just it, it shouldn't degrade performance when put aside many other stuff (and that is what's happening here)

wilkerlucio18:12:09

seems like there is more to this story than I'm noticing here

wilkerlucio18:12:09

as part of the query, I have the normal queries for the fulcro components, but I also have one "open" key there, that contains all the data for the reframe stuff

wilkerlucio18:12:39

but the query for it is simple, just the keyword (just get whatever is there to pass down)

wilkerlucio18:12:43

do you think that could be an issue?

tony.kay18:12:45

I didn’t realize you were having fulcro manage reframe data…I thought you had reframe subs?

wilkerlucio18:12:25

there are many moving parts on this system (yeah, not fun...), so, there is more to the app than this part

wilkerlucio18:12:27

the app is huge

wilkerlucio18:12:33

we are starting on a part of it

wilkerlucio18:12:40

so, there is reframe -> fulcro -> reframe + fulcro

tony.kay18:12:07

Unless you want to learn the internals of reframe’s implementation, I would not mix

tony.kay18:12:16

(or perhaps you can blog about it later) 😄

wilkerlucio18:12:31

I have no way around it... we have to do it gradually

wilkerlucio18:12:58

there is another part of the system that's just fulcro inside reframe, that part runs very well

tony.kay18:12:01

So why not make divs on a page, and mount the apps on the same page?

wilkerlucio18:12:13

you mean multiple fulcro apps?

tony.kay18:12:16

one js file with multiple clients mounted

tony.kay18:12:45

you can have as many new-fulcro-client calls as you want in an app.

wilkerlucio18:12:56

that was the first approach, but if I do that I would have to run multiple queries and manage multiple widgets, so would be getting back on the actual problem we are trying to solve

wilkerlucio18:12:27

the reason to do that is the composed query will be able to optimize because of the long range, if I distribute that I lose this

tony.kay18:12:32

so yeah, you’re going to have to learn how reframe does its internals

tony.kay18:12:04

Fulcro 2.x might show performance problems with “one big map of data for reframe”

tony.kay18:12:12

because it marks basis time on everything

tony.kay18:12:30

there is an open issue to optimize this, but I have not gotten to it

wilkerlucio18:12:54

maybe I can do some work there, I want to upgrade to fulcro 2.0, but I'll probably have to do it on my time

wilkerlucio18:12:05

and I want to finish at least fulcro-inspect before that XD

tony.kay18:12:06

Om used to have a similar problem with path-meta, but it should be fixd

tony.kay18:12:22

flame map is your friend.

tony.kay18:12:28

let me know what you find

wilkerlucio18:12:12

it's talking almost 1 second to the UI to respond (ok, in dev mode, should get a bit better with prod react)

wilkerlucio18:12:37

at least only the fulcro parts are been affected, the reframe is running like before

tony.kay18:12:18

It has to be that extra state. Could be path-meta wasn’t as well-fixed as I thought.

tony.kay18:12:43

unless reframe is taking the time, but only when Fulcro renders

tony.kay18:12:48

that’s possible too

wilkerlucio18:12:53

humm, seems that something about the event handler can be, the response to the click itself is what is taking so long

wilkerlucio18:12:35

it's calling the om/transact! way more times than I expect (I expect 1, and a quick count seems like it's happening 20+ times)

wilkerlucio18:12:29

check all the lines between the purple items, all are om/transact calls for some reason

tony.kay18:12:06

yeah, 1/20th of a second seems like a better real time 🙂

wilkerlucio18:12:51

the same component and action on devcards only triggers the om/transact twice

tony.kay18:12:33

So, on a different topic: defsc I’m tempted to make a few breaking changes for 2.x. Specifically, dropping support for the non-lambda versions of query/ident/initial-state. (leaving the non-lambda version of css stuff)…OR…just make them all be lambdas. The problem is consistency: Once we start making this and props appear from the outside context, I think it should be consistent.

tony.kay18:12:56

but if I do that, it will add to porting effort if you used defsc already in 1.x

tony.kay18:12:47

I won’t back port the changes to 1.x…so it would be a “I know I have to do this to port” thing.

tony.kay18:12:33

I could be a little more friendly and allow this, but it would cause breakage without errors:

(defsc Boo [this {:keys [db/id]} _ _]
  {:ident [:boo/by-id id]
   ...})

tony.kay18:12:28

it makes sense and reads well, but is incompatible with the current more error-checkable syntax of:

(defsc Boo ...
  {:ident [:boo/by-id :db/id] ...})

wilkerlucio18:12:11

for the initial state, I'm ok breaking it

wilkerlucio18:12:49

but I think for the others we might want to keep the non-lamba versions too, the query is a bit rare to need the lamba version

wilkerlucio18:12:25

but the syntax for the initial-state, the current is very non-intuitive (with the keyword on right side pulling from props)

tony.kay18:12:28

yeah, but then it makes it confusing to newer users, and I don’t like that…“when do i use a lambda?”

tony.kay18:12:24

Also, there’s the possibility that I can get rid of the :protocols and add auto-discovery…like this:

(defsc Boo [this props computed children]
  {:query (fn [] ...)
   :some-other-ns.Protocol/method (fn [...] ...) ; in these cases, only `this` is supplied for you.

wilkerlucio18:12:57

protocols are an open way to add more things, I don't think we should remove it

wilkerlucio18:12:06

what you mean by auto-discovery?

tony.kay18:12:48

if I can figure out how to scan for that protocol, then I could put empty methods for the ones you fail to mention

tony.kay18:12:56

like the CSS one…where you don’t want them all, just one

tony.kay18:12:11

and I can still check for arities

tony.kay18:12:23

it makes the whole thing completely consistent

tony.kay18:12:44

there are some common ones I know (like IQuery), but for others, just namespace it

tony.kay18:12:03

that eliminates defui direct use altogether always…just more concise

tony.kay18:12:53

eliminating all of the “magic” of when you can use a lambda vs just data makes that an easiler thing to sell as the primary thing to build your UI with

mitchelkuijpers18:12:58

Btw if you change all the defsc to lambda's will you lose the nice validation?

tony.kay18:12:26

Yes, BUT, the destructuring will propgate to the lambdas, so Cursive, at least, will be able to highlight the errors

wilkerlucio18:12:31

but what about custom protocols? If I want to add some protocol of mine

tony.kay18:12:58

@wilkerlucio If I can figure out how to read the list of protocol methods from the Clojure runtime, I can auto-discover them…that’s the point 🙂

tony.kay18:12:07

it’s an if I don’t know yet

tony.kay18:12:34

@mitchelkuijpers You’re liking the validation, too, I take it 🙂

tony.kay18:12:45

that was one of my original motivations

wilkerlucio18:12:50

I'm not following... I still don't get how I would add a custom protocol if you remove the :protocols

mitchelkuijpers18:12:15

Yes I see it saves a lot of time, but if you remove it we will make our own macro, we want one anyway which also gives all the css classes and validates them

tony.kay18:12:26

@wilkerlucio

(defsc Boo [...]
  {:your-protocol-namespace.ProtocolName/method-name (fn [args] ...)})

wilkerlucio18:12:38

ok, now I got it 🙂

tony.kay18:12:12

If I can autodiscover with arity, I can output any you omit with underscores for params

mitchelkuijpers18:12:01

No we are al spacemacs and vim users

tony.kay18:12:13

ok…then the highlighting isn’t as useful from Cursive 😜

tony.kay18:12:49

@mitchelkuijpers what do you think of the initial-state “magic” in defsc?

mitchelkuijpers18:12:07

We are just moving to 2.0 and started to use that 😛

mitchelkuijpers18:12:39

But like I said do what is better for everyone if I want stuff Iam just going to port it to our own codebase

tony.kay18:12:18

I’m trying to gather feedback…I do plan on doing what is best, but I’m not sure what that is 🙂

mitchelkuijpers18:12:06

We actually like the magic, because we can do less things wrong which kinda helps 😛

tony.kay18:12:20

ok…but not using the intiial state magic yet?

mitchelkuijpers18:12:11

That it looks at the queries you mean and does the right thing? is that the magic?

tony.kay18:12:40

no…the initial-state calling get-initial-state for you

mitchelkuijpers18:12:09

Yes that part, with the params

mitchelkuijpers18:12:14

Yeah we do use that

tony.kay18:12:42

{:initial-state {:people [{} {} {}]}} means (initial-state [_ _] {:people (mapv (partial Person %) [{} {} {}])} (roughly)

tony.kay18:12:56

and it figures out Person from the query

tony.kay18:12:14

so, if you don’t give me a query in data form, I cannot even generate that

mitchelkuijpers18:12:34

But we started on it yesterday so we can still change things around 😛

mitchelkuijpers18:12:01

But we like that, makes it all very concise

tony.kay18:12:10

My experience (and Wilker’s) is that it is confusing…and sometimes insufficient.

tony.kay18:12:58

the error checking won’t let you put in things that are not in your query, but sometimes you need to (in root for example).

tony.kay19:12:06

I can (and actually have) made it do both…but as I’m writing the documentation I’m having to say things like “if you write it this way with that over there, then …”

tony.kay19:12:56

for example, the 2.x version at the moment will let you do it either way, but if you do a lambda for query, then it has to force you to do it for initial state.

tony.kay19:12:04

(which it gives a clear error for)

tony.kay19:12:21

@mitchelkuijpers what’s the CSS validation you’re looking for? Is it something others would like?

dyba19:12:27

@wilkerlucio thanks for the tip about fulcro! I have an existing Om app with a backend server in production. I was experimenting with Om Next to see what the migration will be like. And it’s VERY painful. From a cursory read of the Fulcro docs, it seems Fulcro has a server implementation too. Is it possible to use Fulcro without the make-fulcro-server ?

mitchelkuijpers19:12:45

That the css classes that you destructure actually exist

tony.kay19:12:53

OH…I see. in the top args list

tony.kay19:12:21

(defsc Boo [this props computed children css] …)

tony.kay19:12:54

I wanted that the other day as well…but the args order sucks there to keep bw compat

mitchelkuijpers19:12:53

Yeah then you get a lot of [this props _ _ css]

tony.kay19:12:58

ideas? - I could auto-shove them into computed…that could cause collisions, though. - Tolerate the fact that they are after children, and only valid if you have a css entry - Make a diff arg list order when you use css

mitchelkuijpers19:12:47

I would go for the second option tbh, but not sure how this works if you don't use fulcro-css?

mitchelkuijpers19:12:14

And then maybe only add it if you use css in that component that is a good idea

tony.kay19:12:16

it’s fine…the macro generates code, so if you don’t use the option, it doens’t generate code that needs css

tony.kay19:12:46

the rule would be that the 5th arg is only needed (allowed) if you add a :css entry to options

tony.kay19:12:08

it’s how I do the protocol already 🙂

mitchelkuijpers19:12:09

Yeah we would love that change

tony.kay19:12:34

could also put it under a namespaced key of computed:

(defsc Boo [this props {{:keys [classname]} :fulcro-css/css} _ _] ...)

tony.kay19:12:33

whatever namespace makes sense…

mitchelkuijpers19:12:58

Yeah that is also a option

mitchelkuijpers19:12:06

Then you don't have clashes

tony.kay19:12:21

right, and no extra args hanging off in the wind

tony.kay19:12:30

and when we decide to add more (protocols that want destructuring), this is extensible

mitchelkuijpers19:12:08

Yes but maybe add computed under a extra key also then?

mitchelkuijpers19:12:21

Not sure but I like the idea especially with extensibility

tony.kay19:12:35

hm, yeah…computed really is a special thing…munging them essentially means this:

(let [{{:keys [classname]} :fulcro-css/css} (merge {:fulcro-css/css (css/get-classnames)} (om/get-computed this))] 
 ...)
in the generated code

tony.kay19:12:05

it gives priority to computed, whereas css is possibly a lot more common a thing to use

mitchelkuijpers19:12:30

Yeah we use that way more, because we only use fulcro-css

mitchelkuijpers19:12:38

And sometimes you need computed

wilkerlucio19:12:57

@daniel.dyba yes you can, I personally don't use the server stuff of fulcro, just the client

mitchelkuijpers19:12:28

@daniel.dyba pathom from @wilkerlucio is very interesting 😉

tony.kay19:12:30

I tend to lean towards CSS gets special treatment

tony.kay19:12:49

the 5th (optional) arg

mitchelkuijpers19:12:13

Yeah to me that also looks better, the other option does look a bit like a hack

tony.kay19:12:35

yeah a lot of ppl don’t seem to know how to do nested destructuring…not even sure I typed it right there 🙂

tony.kay19:12:53

I kind of rely on the compiler to check my work on those…rarely use it

wilkerlucio19:12:06

I tend to be against adding more and more to arglist... but on this case I think it's ok

tony.kay19:12:23

Yeah, I agree with both statements

tony.kay19:12:58

so then the question is should it be arg 3 or 5? 3 breaks existing code, but “feels better” to me

tony.kay19:12:04

and we’re talking 2.x only here

tony.kay19:12:15

so porting is already a thing ppl have to do

tony.kay19:12:04

could get confusing, though…computed is used a decent amount

mitchelkuijpers19:12:54

It might be fixable with a shell script.. but you could also add it before children. I can imagine not a lot of people use that with react 15 because of all the annoying warnings it gives

tony.kay19:12:22

ah, right…so arg 4 and children bump out..that is better. A lot less breakage

tony.kay19:12:04

@mitchelkuijpers I’m going to let you guys write the validation for it, though…I don’t want to parse garden 🙂

mitchelkuijpers19:12:21

We can fix that for you no problem

tony.kay19:12:17

I think it’s harder than it sounds…you’re running validation at compile time.

tony.kay19:12:38

maybe not…you can run garden on the rules

mitchelkuijpers19:12:40

We'll see even without validation it is a nice

tony.kay19:12:43

and see what keys it spits out

mitchelkuijpers19:12:05

I think in fulcro-css it already needs those keys somehow, can't remember from the top of my mind

tony.kay19:12:08

the “no hard dependency on fulcro-css” has a well-known fix, so prob ok

tony.kay19:12:30

all right, if it occurs to me how to do it easily, I’ll prob do it 🙂

mitchelkuijpers19:12:49

Don't hesitate to ask @timovanderkamp he might know some edge cases

mitchelkuijpers19:12:59

I gotta go, good talk ^^

tony.kay19:12:04

I’m sure there are some. Thanks for the input

tony.kay19:12:32

It just occurred to me that I can make the final arguments of defsc optional so we don’t have all of the underscores lying around 😜

tony.kay20:12:29

@wilkerlucio

user=> (:sigs fulcro.client.primitives/IQuery)
{:query {:name query, :arglists ([this]), :doc "Return the component's unbound static query"}}
Gotta love dynamic languages 🙂

tony.kay20:12:09

defsc protocol support is about to get a bit better, I think…

tony.kay20:12:09

hm…only works if it is cljc, though 😕

tony.kay21:12:29

@grzm does that just give you a figwheel cljs repl, or both?

grzm21:12:03

You end up with just a single figwheel cljs repl

grzm21:12:41

If you do cider-jack-in-clojurescript, you end up with two cljs repls, one figwheel, one not.

grzm22:12:11

Thanks for merging 😉

tony.kay22:12:46

thanks for fixing it 🙂