Fork me on GitHub
#reagent
<
2015-08-05
>
jhchabran09:08:24

I posted the following gist yesterday on #C073DKH9P https://gist.github.com/jhchabran/e09883c3bc1b703a224d Besides point 1 which has nothing to with Reagent , do you guys think it’s correct enough to make a PR to reagent-recipes to update the gmap example ? (According ofc that I replace the subscribe call feeding coordinates with some traditional atom)

escherize12:08:36

I think that'd be awesome.

jhchabran14:08:48

@escherize: I’ve removed the atom holding the gmap, it was necessary only because I didn’t place the run! in the right scope, instead of putting it inside :component-did-mount

jhchabran14:08:25

gonna make the PR, it seems nice enough to me with that correction

antishok14:08:52

@jhchabran: btw just curious, where does re-frame talk about not using reactions in views?

jhchabran14:08:40

@antishok: looking for where it is, IIRC it said to use subs instead

jhchabran15:08:04

@antishok: Mike says it there : https://github.com/Day8/re-frame/issues/102 > reaction should not be used in views (as you have done above). Should only be done in subscriptions. Best to have a look at the examples.

antishok15:08:49

well i think he means not to use the reaction in the render function itself, it's ok to have reactions in the outer setup function (of a "form-2 or 3" component)

antishok15:08:25

and yes it's best if those reactions are subscriptions or based on subscriptions, instead of raw access to app-db

antishok15:08:55

so your comment #1 doesn't seem to really be any violation

jhchabran15:08:33

thanks for the clarification

mikethompson17:08:23

@jhchabran: that Gist looks wrong on a number of levels -- that use of run! look very wrong. I'm dunno about the google maps interface, and i don't have much time ... but a sketch .... First create a wrapper which does the subscription and passes it as props to a child:

(defn wrapper 
    []
    (let [pos  (subscribe [:current-position])]
       [google-map @pos]))
At this point you can create a google-map component which wisely uses its one parameter. You can add lifecycle methods like component-did-update (which will get called when the props change), etc

jhchabran17:08:10

@mikethompson: thanks for your feedback simple_smile google map interface isn’t really important, it was mostly how to call some js functions updating it when my data changes

jhchabran17:08:53

his example seems to address the same needs, thanks for the link

mikethompson17:08:58

Hopefully that nudge about using a wrapper, sending in props, and Nils' code will help you along

mikethompson17:08:01

In re-frame, reaction (and run!) should only be used in subscription handlers. Never in a view.

jhchabran17:08:29

gonna read carefully the blog post

jhchabran18:08:39

damn, I missed this one 😄

mikethompson18:08:59

Eeek. Should have written ...

mikethompson18:08:26

(defn wrapper 
    []
    (let [pos  (subscribe [:current-position])]
       (fn []                 ;; <--- return the renderer
           [google-map @pos])))
Has to be a Form-2 function

jhchabran18:08:18

or it’ll never get evaluated again, ok simple_smile

mikethompson18:08:06

No, previously the subscription would have been destroyed and recreated on every render.

mikethompson18:08:52

But, using Form2, the subscription is created once, in setup part, and the value then flows through to the render function, which might be called many, many times.

antishok19:08:24

what is it that makes using run! there so wrong?

nblumoe20:08:56

@jhchabran @mikethompson just let me know if you have any remarks or questions about the blog article. Just realized it's missing the actual usage of the component.

mikethompson22:08:37

@antishok: as it is, it is a memory leak. That run! will never get cleaned up. And that means the subscription pos (which it uses) will never be cleaned up. Which means as you create more of these map components, you'll get more and more active (but unused) subscriptions. So every time app-db changes, there'll be all this unnecessary processing in the unused, but still-there subscriptions, etc.

mikethompson22:08:27

You may be able to get away with this leak in an app that has only one map component. But it isn't a pattern that will work more generally.

mikethompson23:08:49

@nblumoe: thanks for that blog post, I've pointed quite a few people to it over the time.

mikethompson23:08:27

@nblumoe: in that post, you talk about having problems with vectors of params. I suspect you ran into https://github.com/Day8/re-frame/wiki/Using-%5B%5D-instead-of-%28%29#appendix-2 Although that is a guess, and based on not much information.

mikethompson23:08:58

@nblumoe: and, yes, not supplying the actual props in the function is likely to confuse people. But you have noted it. So they should be clear.