Fork me on GitHub
#om
<
2016-09-02
>
dnolen16:09:28

going to do one more alpha, if I don’t hear any bad news will work on beta next week as well as updating all the GitHub content to point to new stuff instead old stuff

anmonteiro17:09:19

@dnolen hrm I’m not sure about your transform-reads fix

anmonteiro17:09:25

it seems to me that it causes unnecessary reads

dnolen17:09:58

@anmonteiro the original idea was just wrong

anmonteiro17:09:00

whenever it matches a key in the indexer, a tx like [(do/it!) :foo] becomes [(do/it!) :foo {:foo [:transformed]}]

dnolen17:09:08

it should only have ever been additive

dnolen17:09:12

not destructive

dnolen17:09:17

anything else is just optimization

dnolen17:09:22

and I’m not going to work on that now

anmonteiro17:09:05

@dnolen right, I’m happy to provide the optimization patch if you agree that the example above should result in: [(do/it!) {:foo [:transformed]}]

dnolen17:09:29

yes it would be super awesome if this optimization step was a l carte

dnolen17:09:33

thus testable yada yada

anmonteiro17:09:15

I’m worried it might break some code that uses that assumption

anmonteiro17:09:40

e.g. parsers expecting a query and getting nil (as a result of :foo)

dnolen17:09:57

I don’t follow

anmonteiro17:09:27

previously transform-reads would transform [(do/it!) :foo] to [(do/it!) {:foo [:transformed]}]. This implies that the parser for :foo will get [:transformed] as its query currently it will get both :foo (where query in the env is nil) and {:foo [:transformed]}, with the right query

dnolen17:09:32

I just don’t know if I care that people wrote code like that though

anmonteiro17:09:04

well that was the purpose of transform-reads for me I think

anmonteiro17:09:26

that we’d actually get the right query in the parser

anmonteiro17:09:53

@dnolen then again, this problem will be solved with the optimization I mentioned above

anmonteiro17:09:00

so probably not a reason for concern

dnolen17:09:12

yeah I do not like the dependency implied by how it worked before

anmonteiro17:09:04

it just doesn’t make sense to me that specifying :foo to re-read after a transaction doesn’t get transformed into the actual query for :foo

dnolen17:09:21

only if you bake in a lot of assumptions about how someone might construct a transact value

dnolen17:09:31

the way that it worked before just makes testing way harder

dnolen17:09:46

you need a bunch of things you might not even care about to see correct behavior

dnolen17:09:49

and I just don’t like that

dnolen17:09:02

I spent too much time debugging this problem already

dnolen17:09:04

which is not a good sign

anmonteiro17:09:16

@dnolen I can definitely agree to that. I think what I’m suggesting is orthogonal to that concern

dnolen17:09:32

but I just don’t agree that breaking some assumption is bad here

dnolen17:09:37

that assumption wasn’t a good one

dnolen17:09:44

it showed how complected transform-read was

anmonteiro17:09:37

I think it can be summarized as: if we find :foo in the indexer, use its query. If no indexer is available or we can’t find it, use the key that we have at hand

anmonteiro17:09:28

but don’t put both :foo and its transformed query in the result

anmonteiro17:09:54

I’m just concerned because this little change influences every transaction that specifies keys to re-read

dnolen17:09:59

well it’s not like they are both going to appear anyway

dnolen17:09:14

since you get a map the next one will rewrite the first one

dnolen17:09:27

it’s just a map access with no query, so no computational cost

anmonteiro17:09:30

now I’m not following

dnolen17:09:45

reading some key twice can’t possible matter

dnolen17:09:05

unless A) you made some assumption about that key being associated with some query everywhere B) you have side effects during reads (yuck)

anmonteiro17:09:42

C) you return a large data structure that path-meta needs to walk more than one time now 🙂

anmonteiro17:09:47

@dnolen ^ this is the sole reason for my concern about reading more than once

dnolen17:09:22

ok so that’s one that matters 🙂

dnolen17:09:22

optimization

hueyp17:09:34

having transform-reads not be destructive would be awesome

anmonteiro17:09:44

@hueyp already in alpha42 😛

anmonteiro17:09:37

@dnolen I’ll try to put something together tonight

anmonteiro17:09:20

@dnolen I had one more optimization idea for the parser. I’m pretty sure we don’t need to call path-meta when doing recursive parsing

anmonteiro17:09:36

so the parser we feed to env should ideally have elide-paths true

anmonteiro17:09:58

I think we really just need to call path-meta on what’s returned at the top-level

anmonteiro17:09:13

since other metadata stuff will be overwritten anyways

hueyp17:09:14

thats what we do — is recursive parsing baked in now?

hueyp17:09:26

(the elide-paths on recurses)

anmonteiro17:09:33

@hueyp nope, it’s what I’m proposing

nickt18:09:33

hey all. What's wrong with this component?

(defn button [props & children]
  (reify
    om/IRender
    (render [_]
      (dom/button #js
        {:className "my-button"}
        children))))

nickt18:09:53

it renders a bunch of weird <span>s when I (button nil "+")

nickt18:09:17

also how would I merge that #js {:className "my-button"} object into the incoming props?

selfsame18:09:29

@nickt what is children? a string?

selfsame18:09:22

lazy maps of react components should be fine but I think your wanting to (apply str children)

nickt18:09:59

@selfsame well children here would be "+" right?

selfsame18:09:15

yeah sorry that was evident 🙂

nickt18:09:15

if I'm using it as (button nil "+")

nickt18:09:38

right now it doesn't even render the <button>

nickt18:09:45

it just renders a bunch of <span>s

selfsame18:09:55

so (button nil (map #(dom/p nil %) ["+" "-"])) would work (maybe, not sure if button can contain p), but a list of strings will cause issues i think

nickt18:09:25

@selfsame but I can use (dom/button #js {...} "+") just fine?

nickt18:09:35

I was intending to just replicate that API

selfsame18:09:40

yeah let me explain better, (defn button [props & children] (list props children)) would return (nil ("+"))

selfsame18:09:51

react is having trouble rendering a list of strings

selfsame18:09:47

so maybe try (defn button [props text] ) or flatten the children list into a string

selfsame18:09:55

and just to be on the same page, the & variadic arg will always result in a list

nickt18:09:21

seems (defn button [props text]) yields the same issue

selfsame18:09:40

ok spinning up a repl with om 😉

nickt18:09:45

this is the output I'm getting:

<span data-reactid=".0.0.1:$text:0">+</span><span data-reactid=".0.0.1:$cljs$lang$protocol_mask$partition0$:0">393216</span><span data-reactid=".0.0.1:$cljs$lang$protocol_mask$partition1$:0">0</span>

selfsame18:09:26

you're on current gen om, not om.next right?

nickt18:09:39

yea om 0.9.0

anmonteiro18:09:18

@nickt even if you’re using om.core, you can use the latest alphas

anmonteiro18:09:25

you get React updates if nothing else

nickt18:09:50

tbh doesn't really matter for what I'm doing, I'm happy to stay at the old one

selfsame18:09:30

ok so a om.now component has two fields

(fn [data owner]
    (reify
      om/IRender
      (render [_]
        (dom/p nil (:text data)))))

selfsame18:09:01

I think you want to use a plain fn

selfsame18:09:26

(defn button [props text]
  (dom/button #js  {}
    text))

selfsame18:09:45

which you can use inside of component renders

nickt18:09:57

what if my button component starts to involve it's own state?

nickt18:09:59

(for example)

nickt18:09:04

also, how would I merge the incoming props into that #js {} object?

nickt18:09:19

(so that I could override a className, for example)

selfsame18:09:22

heh, trying to get back in the om.now mindset.. the first field is usually an om cursor, which is a extended clojure map or vector, but you can provide om/build with whatever you want for it

selfsame18:09:28

you can also provide data in https://github.com/omcljs/om/wiki/Documentation#build via :state :init-state and :opts

selfsame18:09:47

(defn button [data owner]
  (reify
    om/IRender
    (render [_]
      (dom/button #js
        {:className "my-button"}
        (om/get-state owner :text)))))
;usage
(om/build button {} {:init-state {:text "+"}})

selfsame18:09:54

I think that's valid

nickt18:09:33

@selfsame cool I'll try that

nickt18:09:47

wait what about passing my own props?

nickt18:09:05

like, in react itself, I could write a Button component with exactly the same API as <button>

nickt18:09:38

class Button extends Component {
  render() {
    return <button {...this.props}>{this.props.children}</button>;
  }
}

nickt18:09:55

<Button onClick={whatever} className="hi">Click Me!</Button>

nickt18:09:18

that's all I'm trying to do here,

anmonteiro18:09:25

@dnolen question regarding the transform-reads patch: (transform-reads r [:foo :bar]) where we can find a query [{:foo [:bar]}] should return which? 1. [{:foo [:bar]}] 2. [{:foo [:bar]} :bar]

dnolen18:09:18

@anmonteiro I’m inclined to optimize left to right

anmonteiro18:09:26

Before your fix it would return [{:foo [:bar]} {:foo [:bar]}]

dnolen18:09:37

I understand now

anmonteiro18:09:48

now it returns [:foo {:foo [:bar]} :bar {:foo [:bar]}]

anmonteiro18:09:09

I’m inclined to say 1.

selfsame18:09:51

@nickt well om.now is a layer of abstraction over the react props, so the idea is you pass data via the reified fn's first arg or in the third arg with :opts :state or :init-state, same concept as react's props

nickt18:09:58

@selfsame hm ok. So using your example above, how do I merge props onto one another?

nickt18:09:10

i.e. the button gets an onCclick prop, but I also want to supply my "my-button" class naemn

selfsame18:09:25

(om/build button {:js-style {:background "red"}}) and in button (dom/button #js {:style (clj->js (merge (:js-style data) {:color "white})))

nickt18:09:34

ok i got it!

selfsame18:09:50

@nickt great, welcome to the party 🙂

nickt18:09:12

om/clojure/clojurescript is no easy task to wrap your head around

nickt18:09:24

but it's a really exciting language/ecosystem

anmonteiro19:09:38

@dnolen actually hold off on merging, adding one more test

anmonteiro19:09:00

the user might want to specify the same mutation twice, and I think I’m being too restrictive by using a set

anmonteiro19:09:42

and it’s probably incorrect anyway as the set doesn’t guarantee order

anmonteiro19:09:50

so reads might happen before the mutation

dnolen19:09:56

@anmonteiro yeah have to keep the order

anmonteiro19:09:35

@dnolen k updated the PR

anmonteiro20:09:31

@dnolen you mentioned a while ago wanting to merge Cellophane into Om

anmonteiro20:09:52

which release would that be targeting, since you’re now thinking about Beta

dnolen20:09:17

@anmonteiro yes would be nice for that to get in before whatever the “final” release is

anmonteiro20:09:57

@dnolen OK I can probably start putting together a branch that includes that migration

anmonteiro20:09:09

and we’d schedule it to be merged after you cut beta1 or something

anmonteiro20:09:19

such that it’s still not a part of that release

anmonteiro20:09:32

@dnolen there’s one more thing. Would you be against having a build.boot file in Om? It could either coexist with project.clj or even be a total migration I’m just asking because migrating to Boot in some of my projects has been a total productivity booster

anmonteiro20:09:48

including automatically running tests on file save, etc

dnolen20:09:58

not against that no

anmonteiro20:09:10

awesome. which option?

anmonteiro20:09:28

coexisting with project.clj or just totally migrate?

anmonteiro20:09:31

it’ll definitely make it easier to merge Cellophane in

dnolen20:09:40

coexisting is preferred