Fork me on GitHub
#om
<
2015-11-06
>
dnolen00:11:26

@adamfrey your simple question had way bigger ramifications than I originally considered

dnolen00:11:24

in a good way! simple_smile

leontalbot03:11:33

Hello guys, hello @dnolen , so I took time to clean up a bit, https://gist.github.com/leontalbot/1d57bb05c063a989757c

leontalbot03:11:58

when I toggle first question radio button I get :

leontalbot03:11:26

[ 11.488s] [om.next] #object[votepourca.omnexttest.Answer] transacted '[(selections/update-radio {:value value, :name name})], #uuid "35d87a1d-a354-4dc3-9bea-967ab9c6e470"
next.cljs?rel=1446568734541:905

leontalbot03:11:29

Uncaught #error {:message "No queries exist for component path (votepourca.omnexttest/Form votepourca.omnexttest/Question votepourca.omnexttest/Answer)", :data {:type :om.next/no-queries}}

leontalbot03:11:02

And the mutation doesn't do anything.

leontalbot03:11:05

Anyone? probably a newbie mistake but can't find it!

leontalbot03:11:10

Thanks in advance!!

leontalbot03:11:57

(Note: when I click a second time on any radio button, I don't get the error message, only the transacted trace, but again nothing changes.)

leontalbot05:11:37

I updated the example

leontalbot05:11:40

by adding a subquery :a -> {:a ~subquery} for answers in Question UI I don't get the error message

(defui Question
  static om/Ident
  (ident [this {:keys [id]}]
         [:questions/by-id id])

  static om/IQuery
  (query [this]
         (let [subquery (om/get-query Answer)]
           `[:id :type :label {:a ~subquery}]))

leontalbot05:11:16

But the mutation doesn't work, and I loose my radio button props such as type or name so that I when I toggle a radio button, it changes in regular text input...

jdubie06:11:26

i’m trying to build a simple example that uses om/subquery to fetch data for the currently displayed view. this seems very similar to @bplatz’s question about an idiomatic way to build up queries for only rendered components. Basically on demand fetching for only what’s visible. the example linked to below has a mutation that modifies :route then renders a different component. at this point the repl shows the Roots query has been updated but it doesn’t re-render. i admit it seems dumb to have some state change, render, then render again because your query has changed because you mounted a child ref’ed by a subquery. however, directly changing the query directly isn’t a good option because i want an unrelated component to be able to update :route via a mutation and cause this component to render a different view. interesting pieces from example here: https://github.com/jdubie/om-next-router-example/blob/master/src/om_router/core.cljs#L116 https://github.com/jdubie/om-next-router-example/blob/master/src/om_router/core.cljs#L136-L141

simonb06:11:36

What's the right command to build om-next-demo?

simonb07:11:05

I have an app which can be viewed in multiple languages (English/Chinese). So I store the current language in the Root view - in local state (initLocalState?). Or should that be shared? How do I ripple the selected language down to the child views? In props, query params, shared? Help! Thanks!

steveb8n09:11:39

@leontalbot: I reported the same in an earlier comment. I think there might still be an issue with components 3+ levels deep but a simple solution is not to transact from the Answer component. Instead make a callback to a computed fn from Question. This should fix it.

steveb8n09:11:13

take a look at my description from earlier this week for more details since there I used a slightly different fix

dnolen11:11:08

@simonb: sorry don’t have time to explain how to get om-next-demo running. Sounds like you want to look at :shared which I fixed up yesterday. Not in a release yet though. You will need to install from master.

dnolen11:11:16

@jdubie: example is too long for me look at sorry

dnolen11:11:37

if people actually want me to look at stuff minimize it to the actual problem

dnolen11:11:02

most problems can be described with about as much code as I use in my tutorials

dnolen11:11:04

which is nothing at all

dnolen11:11:02

@leontalbot: your example is also still too cluttered for me to consider

hmadelaine11:11:23

Hello ! Is it me or there is a problem in the last Om-Next cut with the :shared and :shared-fn options ? There are optional but it fails at the expression :

(when (contains? config :shared-fn)
                                                 ((:shared-fn config) data)))

dnolen11:11:50

@hmadelaine: hrm what’s the bug there?

hmadelaine11:11:13

@dnolen Cannot read property 'call' of null

hmadelaine11:11:31

(contains? config :shared-fn) returns true

simonb11:11:42

@dnolen thanks. I'm reading the source. I think I have to use :shared-fn so can I change to reflect (say) a change of language. I think, :shared-fn gets passed props - props are immutable (right?). So, I'm just wondering how to get the change into :shared and how to propagate the change down to the children. Is this right or can I just use :shared (not use :shared-fn) and change the language key in :shared. Thanks!

dnolen11:11:10

@simonb props are immutable values but that doesn’t mean they don’t change over time

dnolen11:11:33

:shared-fn will run again if you change something at the root.

simonb11:11:30

@dnolen can a component modify its own props? For example, the root component is inited with props indicating a language of English. Then the user selects Chinese, how should I get this change into the root so it ripples down to the children? Thanks.

jannis11:11:18

@simonb: om.next/transact! is what you're looking for

jannis11:11:29

@simonb: The root component gets its props via its own query. So you could have it query for [:app/lang ...]. The implement a read method for :app/lang and a mutate method for 'app/set-lang that changes :app/lang in the app state. Then, use :shared-fn (fn [props] {:lang (:app/lang props)}). When you want to change the language in your app (e.g. by clicking on a button), kick off a transaction like this: (om/transact! '[(app/set-lang {:lang "en"}) :app/lang]) (with ' = backtick).

simonb11:11:02

@jannis thanks that's a big help. Is :changed-fn above :shared-fn ?

simonb12:11:12

So, props == query. Is this the same for all components? Does query always get added to props?

bplatz12:11:29

@simonb Yes, if you implement it correctly it is. :shared and om/computed are ways to pass information outside of query if necessary.

bplatz12:11:31

But beyond the root component, you are responsible for passing the proper props down to each component according to their query contract. There isn't magic in there that will do that for you.

jannis12:11:43

@simonb: props = query result (your responsibility to pass down the results from the parent) + computed (for e.g. callbacks, stuff that is not part of the query)

bplatz12:11:25

@jdubie I've been thinking about the best way to do this. I think ideally the router should handle modification of the query, and you sort of need the selection to happen outside of IQuery, as you want the query modified prior to it being passed to the parser. One idea is to use a key identifier for the route node components (similar to naming a interceptor in Pedestal, for example). If the query join in IQuery utilizes the same key, you could dynamically select the parts of the query that are relevant to the current route.

bplatz12:11:18

But it is a problem I feel is necessary to solve, at least for my app, and so far the methods to do so feel hacky and confusing... so I'm still working on figuring out the best way.

bplatz12:11:58

The method I'm describing would require a route component for each node in a nested route. You'd want the query joins to be exclusive of each other, and no other parts to the query except the mutually exclusive subcomponents.

dnolen12:11:10

@bplatz: this sounds overly complicated to me

dnolen12:11:51

if the queries are dramatically different and need coordination why even bother changing queries over just changing components

dnolen12:11:17

in general it’s better to talk about problems so that they can be understood

dnolen12:11:36

over whatever solution you’re mulling about Om Next and your interpretation of the solution

dnolen12:11:13

it just makes too hard to understand if there’s something missing or whether it’s another documentation issue

bplatz12:11:11

In the simplest of terms I can produce, the need is a nested data-driven router that only selects the bits of query that are relevant for the current route.

simonb12:11:33

@bplatz & @jannis thanks. So, env in the read method of the parser is props? Or includes props? I'm trying to understand the query contract's relation to props on components. query returns a datalog expression, which gets passed to read, read also can access props for the component. Is that right? Thanks.

dnolen12:11:09

@bplatz: right but I don’t understand why you need this - what problem is this solving?

bplatz12:11:46

@simonb No, it is map that has the things you pass into it allowing you to get the props you need, based on the IQuery you declare.

dnolen12:11:07

(this is an honest question btw, just trying to understand what actual advantage you are trying to achieve)

bplatz12:11:56

@dnolen I come from the enterprise software space, so our apps have everything from analytics, workflow management, financials, etc. I simply want to ensure the entire app isn't loaded. The root component query, without further manipulation, contains everything the app could ever do.

bplatz12:11:25

Some data sets are large, and if not needed I want to ensure they are not loaded.

dnolen12:11:15

right so this is far into the category of things that Om Next does not care about and will never care about

dnolen12:11:35

this is about a problem that you’ve constructed

dnolen12:11:44

nothing fundamental

dnolen12:11:53

that said ...

dnolen12:11:10

I would not try to get this done with just the primitives

dnolen12:11:34

why not provide you own root class thing over defui that automates all the bits you will need

dnolen12:11:46

this is a problem I actually care about - are the Om primitives composable enough to accomplish higher level framework goals / abstractions

dnolen13:11:55

@bplatz: another possibility is that you just want a delay primitive

dnolen13:11:12

so you can just specify total queries and then force some part of the query to load

simonb13:11:11

@bplatz: I think (from reading the om source) it includes [:state :shared :parser :logger]

bplatz13:11:22

@dnolen: my own root class is what I proposed above as a router, it would manipulate query from the outside.

bplatz13:11:41

A delay I think would be a nice addition, however for this particular case it may not be enough. Just the query graph itself could be significant, and it seems streamlining that to the relevant bits would provide more efficiency. But I don't really know yet, as none of this has been tried. simple_smile

bplatz13:11:11

@simonb: Yes, and :state will contain your data, which you provide. I think a read through the wiki overviews is the best way to provide you context for this. I'd tackle that before reviewing source.

dnolen13:11:11

@bplatz: sorry for not being clear

dnolen13:11:22

I don’t mean root class as in the root of component tree

dnolen13:11:34

I mean root class as in new component type

dnolen13:11:57

a component type that knows how to be coordinated by the root

dnolen13:11:09

in the way that you are suggesting

a.espolov13:11:30

@dnolen: I have two questions for you. 1. No ideas how to use mixins let React with om. next, whether such a question? 2. Use of third-party components React?

dnolen13:11:56

@a.espolov: not interested in mixins

dnolen13:11:19

whatever people use those for, figure it out another way, protocols, specify etc.

dnolen13:11:32

3rd party components should work fine

bplatz13:11:38

@dnolen I think think more dynamism in IQuery would be ideal. Perhaps if you could also return a function with just :shared passed in, then you'd be able to put your route information into :shared and return just the parts of the query necessary. You shot down the idea, and probably for some reasons that are not clear to me, but in the interest of describing what could be a simple solution to me, that is it.

dnolen13:11:30

@bplatz: right because you offered a solution to a problem I don’t understand

dnolen13:11:54

in general not going to spend much time on feature ideas if I cannot understand the problem

dnolen13:11:32

but I’m happy to work out what the real underlying problem is and come up with general solutions that cover a wider set of use cases

dnolen13:11:38

@bplatz: I also do not even understand what you’ve suggested about :shared simple_smile

dnolen13:11:00

but as long as thing keep getting framed in terms of solutions I suspect I will continue to have conceptual difficulties where I would like to be of some help.

bplatz13:11:05

OK, that is a reasonable stance. I think the problem has been beat down enough and is understood, but please correct me if I'm wrong. I'm thinking in solutions now because I need to come up with one. If I think of any more parts to the problem or other use cases I'll send them your way.

dnolen13:11:12

@bplatz: no I still don’t understand the problem at all

dnolen13:11:48

because I’m struggling to find the underlying issue filtered through your current understanding of Om Next and the various straws you are grabbing at (which is fully to be expected)

dnolen13:11:47

I also haven’t seen any code at this point simple_smile

dnolen13:11:03

it seems you could write at most 30 lines of code to demonstrate the issue

bplatz13:11:29

OK, I may just lack a better way of explaining it then. It is the ability to restrict the query graph to the parts that are relevant to the current UI state (or route). Same problem that Google Closure Modules solves for the relevant parts of your JavaScript code, which I've seen you produce great commentary about.

dnolen13:11:58

so it just sounds like you want the root to be in control the query

dnolen13:11:08

then why bother distributing it to components at all

dnolen13:11:20

that seems to me root of all of your problems

bplatz13:11:02

[{route-a-component (om/get-query RouteA)} {route-b-component (om/get-query RouteB)} {route-c-component (om/get-query RouteB)}]

bplatz13:11:18

Only pick up one of the three.

dnolen13:11:07

why bother with this?

dnolen13:11:19

if this creates problems for how you want to build your app?

dnolen13:11:19

absolutely nothing about Om Next says you have to build your query in this way - the docs just show the expected primary useage.

dnolen13:11:03

as in why can’t the root have it’s own query building mechanism?

dnolen13:11:25

it’s just data for whopping good reason

dnolen13:11:35

there’s no syntax restricting you

bplatz14:11:49

I want to distribute it to the components for the same value proposition you state for Om.Next. There is a lot of ease and power in the (om/get-query RouteA), which by itself is a nested query graph. The only thing is I just need one of RouteA, RouteB, or RouteC at a given time.

dnolen14:11:28

@bplatz: you’re still missing what I’m saying

dnolen14:11:37

you can put queries on the components

dnolen14:11:47

but how you build the master query

dnolen14:11:00

nothing prevent you from building the total query at the root for the whole thing

dnolen14:11:08

and have the root component supply a filtering thing

dnolen14:11:20

which walks the total query removing all the stuff that doesn’t matter

bplatz14:11:54

Which is exactly what I suggested I was doing in response to @jdubie and you then stated I was over complicating things and henceforth this conversation.

dnolen14:11:34

@bplatz: reading what you wrote that’s not the idea that I read

dnolen14:11:49

but it’s easy to misunderstand

bplatz14:11:35

I think if there is not a more native support of this sort of thing directly in Om.Next, then a router would take the responsibility of filtering the query, as it should know the route.

bplatz14:11:18

By aligning routes with query components, i.e. /index.html = :route-component-b, then you could use those keys to filter the query nodes.

dnolen14:11:23

@bplatz: we’re not going to add any routing abstraction like that. But also important to not create complications for those who wish to do so.

abp14:11:14

isn't routing basically union? can be this or those components and i only render one?

dnolen14:11:21

but based on your earlier statement - which again I don’t fully understand - there seem to be elements that aren’t needed (as far as I tell at the moment)

dnolen14:11:36

I would not introduce routing components or anything like that

abp14:11:44

sorry for coming with some hand wavy half knowledge shouting into you convo

dnolen14:11:46

I would implement this simply as a filter on the root query

dnolen14:11:11

but you may have determined a reason why this doesn’t work which you haven’t yet stated

bplatz14:11:02

I mentioned a route component, yes. I thought there might be other logic required at a nested route node, but I was/am thinking through what would be required for my app.

bplatz14:11:20

The component would be a simple reusable part that knew were to insert a nested route inside the dom/component tree, but that part is outside the scope of IQuery filtering.

bplatz14:11:55

It was focused on render.

dnolen14:11:20

Om 1.0.0-alpha15 released, includes some tweaks you can tire kick :shared and :shared-fn now

bplatz14:11:54

@dnolen: no reason yet that it will not work, that is the path I'm headed down and I'll say something if I run into an obstacle.

dnolen14:11:11

@bplatz: cool, sounds good

dnolen14:11:25

@bplatz: off the cuff, the primary issue will probably be performance

dnolen14:11:51

for this use case where the root wants to control the query it may be interesting to actually diff the query itself

dnolen14:11:17

to figure what needs to change

bplatz14:11:56

When considering a typical navigation structure, most of the differentiation happens near the root (your main nav). It is possible limiting the scope to just that is "enough" to reduce the query down, as opposed to having to be precise all the way down to the furthest leaf.

dnolen14:11:17

@bplatz: right it might not be a perf problem at all.

wilkerlucio14:11:14

@dnolen: talking about perf, do you think Om.next is suitable for apps with constant animations (like Musicoacher)?

dnolen14:11:59

being as fast or faster than React is still a goal

dnolen14:11:31

“naive” React is what I mean of course

wilkerlucio14:11:52

cool, thanks for info

dnolen14:11:35

just released 1.0.0-alpha16

dnolen14:11:53

included fixes so that remote integrate with DataScript is feasible

dnolen14:11:04

i.e. transacting remote pulled entity tree (from Datomic) into DataScript

dnolen15:11:01

also the last two releases include devcards integration

dnolen15:11:15

it’s just a start but already pretty cool

dnolen15:11:43

for example here are three of the tutorials as defcards

dnolen15:11:52

makes it much simpler for me to see regressions at a glance simple_smile

iwillig15:11:20

thats pretty cool @dnolen

dnolen15:11:08

@iwillig yeah @jannis has already demonstrated how easy it is test components in isolation

dnolen15:11:24

this gives you the beginning of reconciler mocking

dnolen15:11:54

would like to get devcards history control hooked up here as well as better way to visualize the state

dnolen15:11:48

but this is already a much better development / testing setup than I ever had in the past

jannis15:11:09

@dnolen: I just pulled master and tried lein cljsbuild once dev. It fails with this error: Exception in thread "main" java.lang.AbstractMethodError: cljsbuild.compiler.SourcePaths._find_sources(Ljava/lang/Object;)Ljava/lang/Object;, compiling:(/tmp/form-init38129677347927342.clj:1:71)

dnolen15:11:11

It depends on latest ClojureScript, you need to bump your tooling

dnolen15:11:45

See the 1.7.170 announce

jannis16:11:08

Would it make sense to bump the version of lein-cljsbuild in project.clj?

jannis16:11:48

I've bumped that to 1.1.1-SNAPSHOT and now I get java.lang.AssertionError: No ns form found in /home/jannis/oss/om/src/main/deps.cljs. What version of lein-cljsbuild. Probably still missing something at my end.

dnolen16:11:55

you need 1.1.1-SNAPSHOT

dnolen16:11:12

make sure you clean your build

dnolen16:11:45

that error makes no sense

dnolen16:11:52

would need to see the stacktrace

jannis16:11:17

Did that with git clean -xdf before running lein cljsbuild once dev. I'll paste the backtrace somewhere.

dnolen16:11:12

@jannis just looks like a cljsbuild bug it’s trying to load deps.cljs for some reason

dnolen16:11:14

no idea why

dnolen16:11:34

I would report that to the lein-cljsbuild repo

jannis16:11:44

Ah. Running lein cljsbuild isn't even needed to run the devcards. scripts/figwheel.clj is what I was looking for.

thheller16:11:05

well, without the jar part .. but still a source-path seems to be wrong

thheller16:11:56

deps.cljs must be at a source root, it will otherwise be interpreted as a normal cljs ns

thheller16:11:58

which it isn't

jannis16:11:35

project.clj looks a bit outdated. It still refers to "src/dev". And perhaps deps.cljs can go away and be replaced by an {:externs [...]} compiler option in the "dev" build.

thheller16:11:16

@jannis check the source paths of your build

thheller16:11:32

it probably contains /home/jannis/oss/om/src when it should contains /home/jannis/oss/om/src/main

jannis16:11:21

@thheller: No, the cljsbuild build contains {:source-paths ["src/main" "src/dev"]}: https://github.com/omcljs/om/blob/master/project.clj#L30

thheller16:11:22

hmm maybe cljsbuild picks up paths from another target

thheller16:11:28

the other targets still contains "src"

bplatz16:11:45

FWIW, I was receiving a ICollection protocol error on alpha 15/16 and narrowed it down to me using the stubbed out :shared with an atom prior to you adding enhanced support for it. I bring it up because the stack trace doesn't do much to help isolate it the issue. An assertion for map? on whatever is put into :shared would have helped me, although with the enhanced support I would not have put an atom in there to begin with.

dnolen16:11:19

wow lein is so annoying in this regard

dnolen16:11:26

should be inclusions not exclusions

dnolen16:11:35

anyways deployed 1.0.0-alpha17

dnolen16:11:50

which removes all these unnecessary files

jdubie16:11:14

@bplatz: here is an implementation of what you described. a) union query in the “routing” component and b) parse-time filtering of the query based on current route. a) https://github.com/jdubie/om-next-router-example/blob/master/src/om_router/core.cljs#L111-L130 b) https://github.com/jdubie/om-next-router-example/blob/master/src/om_router/core.cljs#L43-L47

thheller16:11:28

@dnolen the problem is probably that there are some build targets still specifying :source-paths ["src" "test"] which is incorrect

thheller16:11:43

but cljsbuild gathers the all together when running jar

thheller16:11:07

I see no reason why these files would be on there otherwise

jannis16:11:17

@dnolen: It builds after making this change on top of alpha17: https://gist.github.com/Jannis/307e03a461e9b7cc26e3

dnolen16:11:47

@jannis changing the :source-paths bit doesn’t make any sense to me

dnolen16:11:08

it's superceded by jar-exclusions

jannis16:11:46

@dnolen: It doesn't? There is no src/dev in the repo anymore, so lein cljsbuild once dev will fail, not finding it. I guess lein cljsbuild doesn't know about jar-exclusions?

dnolen16:11:15

@jannis I don’t know what you are talking about

dnolen16:11:31

if you’re talking about running cljsbuild commands from with in the Om repo

dnolen16:11:40

that’s not something I’m concerned with at the moment

jannis16:11:54

Ah. Yes, I am. They are still mentioned in README.md so I supposed they are expected to work. But then again, the dev task is really not that interesting. The examples still build fine.

dnolen16:11:42

@jannis yeah I’m pretty sure I’m going to convert all those examples into devcards

dnolen16:11:50

how that’s handled today is very tedious and error prone.

dnolen16:11:59

but that’ll probably happen after beta

jannis16:11:49

Cool. I agree, that's a good way forward simple_smile

jannis16:11:01

I'll shut up then simple_smile

flipmokid17:11:48

Hi all, I have an app running with om next which works nicely without any optimizations. However when I stick in :optimizations :simple I get an error for one particular user interaction which says "Assert failed: (reconciler? reconciler)". Does this scream out user error? If so is there anything in particular I should be watching out for?

flipmokid17:11:27

Apologies if this is in the wrong place or too easy to solve!

dnolen17:11:33

@jannis yeah that won’t work you should read over @bhauman and I’s thread from a day ago in #C03S1L9DN

dnolen17:11:41

the state stuff isn’t going to work for Om Next

dnolen17:11:06

@jannis if you have questions you should ask @bhauman I think he understands the issues pretty well

jannis18:11:21

@dnolen: I skimmed through it yesterday or so. I thought one of the issues was the absence of a "history hook". I don't fully understand why but the above snippet seems to make that work. It probably swap!s the state returned by (om/app-state ...) and when defcard then rerenders the RootView (it calls the anonymous function whenever the card state changes), the reconciler has that state set. Not the cleanest way I'm sure but it seems to work.

dnolen18:11:55

@jannis it really cannot be made to work with the state feature at the moment

dnolen18:11:04

Om Next needs to control the root

dnolen18:11:13

and if devcards is feeding it state the whole thing breaks

jannis18:11:54

You mean calling swap! on the app state held by the reconciler breaks things?

dnolen18:11:53

yes devcards listening on the state atom is just broken for Om Next

dnolen18:11:58

not interested in hacking that to make it work either

dnolen18:11:20

so we’ll probably need some kind of custom card - this is what @bhauman suggested

jannis18:11:09

Hmm, ok. Both tutorial examples work with the above trick, including history, so I thought that could be the solution. Probably not then 😕

leontalbot19:11:17

@dnolen, ok now this is under 150 lines, maybe enough short? https://gist.github.com/leontalbot/1d57bb05c063a989757c

leontalbot19:11:09

the mutation does change the state, but the view doesn't update 😞

leontalbot19:11:33

@steveb8n Thanks! in the updated link, I do the transact at the second level now. Still the view doesn't update 😕

jannis19:11:33

@leontalbot: Move the :poll/questions out of the () in the transact! call.

jannis19:11:18

[(selections/update-radio {:name ~id :value ~value}) :poll/questions] is what you want to pass in.

leontalbot19:11:01

thanks a lot!

leontalbot19:11:05

any other advices?

jannis19:11:50

@leontalbot: Just minor things about coding style. Like: no need for apply in (apply dom/div nil (map question questions)) and perhaps (for [x a] ...) would be more readable than (map (fn [...] ...) a) in render of Question, could use more descriptive identifiers for answers than 1, 2, 3, 4, 5 (e.g. :not-interested, :yes-please) but the Om side of things look good to me.

tony.kay20:11:21

@jannis: be careful about dropping apply with lazy seqs...works in dev mode, not in production

scriptor20:11:29

anyone know if repeated browser requests for favicon.ico and om rerenders might at all be related?

tony.kay20:11:32

(from what I've seen on earlier, at least)

tony.kay20:11:42

using mapv is fine, since it isn't lazy

scriptor20:11:44

(this is in om.old)

jannis20:11:57

@tony.kay: What do you mean by "dev" and "production" mode? Optimizations?

dnolen20:11:36

hrm suprisingly neither Relay nor Falcor seem to deal with the temp id problem?

adamfrey20:11:16

weird. Seems like nearly everyone will hit that?

adamfrey20:11:38

or anyone relying on db generated ids, that is.

thheller20:11:00

@jannis using the minified version of React causes different behavior in regard to lazy-seqs, some bindings om.next uses will be incorrect which may cause errors

thheller20:11:05

@jannis (doall (for ...)) or mapv like tony suggested prevents that

dnolen20:11:11

@thheller: would like to see a reproducer for that

dnolen20:11:27

I haven’t seen that in the any of the examples that I wrote with React with advanced optimization on

thheller20:11:29

just include the minified version of react

thheller20:11:34

and use a for

dnolen20:11:40

already did that many times

thheller20:11:43

or any lazy seq really

dnolen20:11:49

for loops, map etc.

thheller20:11:01

I have only seen apply in your examples

dnolen20:11:28

ah yeah you cannot return lazy seqs React doesn’t support that

dnolen20:11:39

children can be an ES6 iterator

dnolen20:11:45

but you must set this explicitly

thheller20:11:54

yeah (doall (for ...)) works

dnolen20:11:59

and Om doesn’t not expose this in any way at all

thheller20:11:01

(for ...) does in dev mode

thheller20:11:11

@dnolen no it works out of the box

dnolen20:11:33

unspecified behavior far as I know

dnolen20:11:49

unless you’re talking about this signature - and I have not seen this documented

dnolen21:11:04

(ctor props Array|Iterator)

thheller21:11:08

no the ES6 iterator is official as far as I can tell from the code

thheller21:11:35

and all my tests confirm that

dnolen21:11:37

I’d don’t really care what the code says

dnolen21:11:41

I only care what React docs say

dnolen21:11:05

if something says you can return a ES6 iterator as element that’s news to me

dnolen21:11:19

children is a different matter

thheller21:11:54

not saying you can return it as an element

dnolen21:11:02

then what are you talking about?

thheller21:11:07

but it can be ONE of the children

dnolen21:11:16

(ctor props Element…)

dnolen21:11:19

that’s the only legal thing

dnolen21:11:34

then you are saying you can return it as a valid Element

dnolen21:11:40

and I haven’t seen anyone say you can do this

thheller21:11:41

(div {} (h1) (doall (for ...)) (div)) works

dnolen21:11:52

that’s what I’m saying

dnolen21:11:01

you are returning an iterator as a React element

dnolen21:11:24

maybe that is supposed to work but I just haven’t seen anyone say this allowed

dnolen21:11:32

or if this is weird feature to support JSX

thheller21:11:07

no idea other than that is makes sense to support it this way

thheller21:11:19

and it all works perfectly as long as you are careful with lazy-seqs

dnolen21:11:19

yeah I don’t care if it “make sense"

dnolen21:11:32

show me the docs otherwise you writing code Om doesn’t support anyway

thheller21:11:25

@dnolen I have no interest in convincing you whether something is official or not

thheller21:11:44

I was just warning @jannis about lazy-seq, just like @tony.kay

thheller21:11:11

if your solution is to tell people to use apply I am fine with that

dnolen21:11:28

right my point is to separate out weird stuff like context or willReceiveProps

dnolen21:11:38

to reduce confusion about what parts of React are relevant

thheller21:11:35

not sure how that is related

dnolen21:11:45

anyways my point is you can only return elements in Om

dnolen21:11:01

I can’t find any documentation on what the semantics of ES6 iterators are in React

dnolen21:11:09

other than children

dnolen21:11:24

but Om doesn’t let you just set that

dnolen21:11:47

anything suggestions other than these facts is just muddying the waters

thheller21:11:01

yes and I'm saying that [child1, child2, iterable, child3] also works

thheller21:11:10

and many people are using that fact already

thheller21:11:38

and that is a React thing, om is not affected

thheller21:11:45

or doesn't care for that matter

thheller21:11:39

just got to be careful with it is lazy, due to different behavior in react.js and react.min.js

thheller21:11:35

that is due to react.js verifying the children immediately (thus forcing the lazy seq) ... and react.min.js not doing this and thus forcing the lazy-seq outside render

thheller21:11:45

which breaks the binding om.next uses in render

thheller21:11:17

I threw away the repo where I verified this ... but you can verify that yourself if you feel like it

thheller21:11:22

that seems to cover it

dnolen21:11:14

not quite, but it did make me look into what React supports which I never actually every used or understood

dnolen21:11:41

prior to that React let you return elements or JavaScript arrays

dnolen21:11:54

I’m assuming the later was present for template splicing

thheller21:11:03

yeah that changed in 0.13

dnolen21:11:08

the arrays can be arbitrarily nested

thheller21:11:21

but need :key

dnolen21:11:33

the ES6 bit is just an extension of something they started permitting

dnolen21:11:31

yeah so this is making me think we should just force lazy seqs

thheller21:11:18

probably a good idea to do that automatically

thheller21:11:42

manually takes a bit of discipline and is easily missed

thheller21:11:04

not sure how to do it efficiently though

dnolen21:11:20

the N is never going to be very big

dnolen21:11:26

so I’m not concerned about that

dnolen21:11:49

already did all my testing with apply which is dog slow, but it’s not the bottleneck

dnolen21:11:51

React is as usual

dnolen21:11:23

@thheller: apologize for being so argumentative. It’s a bit frustrating that React doesn’t state clearly anywhere how it’s supposed to work

dnolen21:11:38

in Om Next I’ve definitely stayed far away from implementation details this time around

dnolen21:11:03

this is just a legitimate Om bug

thheller21:11:57

@dnolen no problem .. I spent more time in the React internals than I care to admit

dnolen21:11:20

yeah there’s a couple places where we dip our toes

dnolen21:11:30

but stuff like that is just a ticking time bomb

dnolen21:11:36

literally every release has broken something

dnolen21:11:00

and I mean internal stuff (breaking public stuff is about progress etc.)

thheller21:11:04

yeah we should just do a cljs react already 😉

dnolen21:11:19

don’t want to work on Android or iOS

thheller21:11:51

hehe yeah me neither

thheller21:11:09

anyways .. if you implement something that forced lazy-seq remember to not throw away microseconds

thheller21:11:49

but React already throws away plenty of them so it probably really doesn't matter

dnolen21:11:47

@thheller: inadvertently you forced feature enhancement since I never knew it was supposed to work this way simple_smile

dnolen21:11:54

now we have a good splicing story

thheller21:11:35

om.dom/* needs to do this as well though

thheller21:11:18

uhm doesn't your test miss one level?

thheller21:11:16

and remember this ALWAYS works with react.js since that already forces all children for you

thheller21:11:28

react.min.js is where things get icky

thheller21:11:56

since it doesn't use ReactElementValidator

thheller21:11:24

which is what forces things

thheller21:11:17

try (for [it [1 2 3] :let [_ (prn [:doing it])]) (dom/div nil it))

thheller21:11:40

the log appears at different points in time

thheller21:11:57

depending on the react version used

dnolen21:11:14

@thheller: ah right om.dom

jannis21:11:08

Really, react.min.js behaves differently from react.js? I thought minifying was about syntax, stripping unused stuff but not changing behavior? Ugh.

thheller21:11:32

@jannis no the dev version of react does a lot of validations to warn you about errors

thheller21:11:41

these get stripped away in the production version

thheller21:11:57

much like :elide-asserts in cljs

jannis21:11:07

@thheller: Well, that's one thing. It's another not to iterate over lazy sequences to collect everything in one version but not do it another, even if that is just a side-effect of stripping validations. Boo. Well, if we can fix that in Om that would be awesome.

thheller21:11:35

well, React doesn't know what "lazy" means .. so you can't really fault them for that

jannis21:11:53

Hah, okay. You've got a point there. 😉

thheller22:11:36

FWIW I wrote a macro in my stuff for for (eg. (dom/for [it [1 2 3]] ...)) that is just a normal for wrapped in (doall ...)

thheller22:11:41

solved the lazy problem for me 😛

jannis22:11:27

Sounds nice

thheller22:11:06

but I only use that for so it works for me ... doesn't work so well if you use map as well

thheller22:11:34

or anything that is lazy

thheller22:11:16

for just looks nicer to me when the body is always something like (my-comp it)

jannis22:11:30

I'm almost exclusively using for... yeah, definitely looks nicer.

jannis22:11:52

Wouldn't it be nice to have that as forall in CLJ/CLJS?

thheller22:11:18

is the macro

tony.kay22:11:29

@jannis: @thheller: and David just patched it to force children for you...see above

thheller22:11:46

@tony.kay: yeah but it is incomplete 😉

tony.kay22:11:57

oh, did I mis-understand?

thheller22:11:30

no, he is probably working on it right now. need to adjust om.dom as well and make it recursive

tony.kay22:11:57

oh, I see. Yeah, I was just reading the factory/force-children stuff, but yeah I missed the om.dom comment

tony.kay22:11:01

reading too fast

thheller22:11:46

yeah we probably should have opened an issue the last time we talked about this

thheller22:11:46

lost quite a bit of context since then 😛

tony.kay22:11:58

yeah, I was trying to remember that React commit...glad you found it

dnolen22:11:14

@thheller: force-children is recursive unless you’re seeing something I’m not

dnolen22:11:22

but yeah working on the DOM bits

thosmos22:11:44

a modification of a minimal example to get om-next rendering server side: https://github.com/thos37/om-server-rendering

thheller22:11:57

#(cond-> % (seq? %) force-children) (dom/div) fails (seq? %) and won't recur ... but adressed if you add it to all om.dom

dnolen22:11:20

but it’s also wrong

thheller22:11:21

just gotta be careful with any other normal react component

dnolen22:11:26

force-children is wrong, fixing that

dnolen22:11:36

but I already changed all the dom nodes to call it

thheller22:11:41

if say something like react-material-ui is a child of an om component, that won't force anything

dnolen22:11:20

right but I don’t really care about the nested cases

dnolen22:11:33

if you instantiate something and pass it through a regular React thing

dnolen22:11:36

all bets are kinda off

thheller22:11:57

yeah but people are still going to do that

dnolen22:11:59

we can of course support higher order composition as long as your not doing it lazily

dnolen22:11:26

they might, but I’m just less concerned because the problems are much bigger than lazy sequences

thheller22:11:30

hell we all do it every time we use (dom/div nil ...)

dnolen22:11:45

right but that case is covered as I said

dnolen22:11:51

by changing all the dom functions

dnolen22:11:53

and all the dom macros

thheller22:11:09

yeah, just saying there might be react components people use just like dom/div

thheller22:11:14

and those won't be forced

dnolen22:11:23

no but that just not something I care about

thheller22:11:24

so people still need to be aware of the issue

dnolen22:11:27

make factories for that stuff

dnolen22:11:34

and use the same simple thing

dnolen22:11:59

as long as the standard thing works, we can document this issue for integrating 3rd party React stuff

dnolen22:11:20

can probably even provide a factory helper

dnolen22:11:25

to automate this

thheller22:11:31

yeah I just settled on the macro since that always works in all cases

thheller22:11:03

well until you use map

thheller22:11:42

plus it uses less resources 😉

dnolen22:11:34

should be fixed for real now

dnolen23:11:51

yep just verified in a small advanced example

dnolen23:11:54

deploying a new release

dnolen23:11:05

1.0.0-alpha18 released