I dont get this one. I have two factories inside a div, both are instanciating a seperate component. If I give both a unique :keyfn they dont render because of children sharing same key. But if I only give one :keyfn, then it stops complaining. So this does not work:

(def search-query (om/factory SearchQuery {:keyfn :search-query}))
(def search-results (om/factory SearchResults {:keyfn :search-results}))

(def search-query (om/factory SearchQuery {:keyfn :search-query}))
(def search-results (om/factory SearchResults))

What I've noticed is that if React is not given a function to create the key then $null is put on the end of the key that it makes up. $null will be different to say what :search-query returns.

@hlolli ^^

https://github.com/omcljs/om/issues/673 is that related to this issue then @cjmurphy ?

Looking at the om.next api, it appears that factory and build are equivalent.

@dominicm: they are different actually

I'm going on my observations when using devtools, that's all.

build makes instances factory makes a factory fn

@dnolen: Makes sense why the example in that github issue is different to my own using the current om. The fn returned by factory is like having build in an anonymous function?

@dominicm: almost, factory returns a constructor fn which has the same shape as normal React DOM constructor fns

build signature is very different

@hlolli: sounds like it's possible both of the keyfns might return nil so then you would have the same key?

ah, that makes sense. Close enough in utility. Makes sense that issue is reported in both versions.

yeah I need to look more closely at 673 - we used undefined specifically to get React to ignore but maybe something changed in React internally that makes that not work anymore

sorry afk

@dnolen: My understanding based on the log messages is that React is now doing (= undefined undefined) and collapsing the children as being the same. I wonder if this means that React is using something like hasOwnKey instead of (not= component[key] undefined) --- I just realised that I just made a horrible mess of blending js and cljs to make pseudo code. Not sure if that's good or bad.

yes, both return nil, the same key in this case is nil.

@hlolli: k right, so yes probably related to 673, will try to look at that later in the afternoon

ok, just to flood, then this is the error message. @dnolen cool!

Encountered two children with the same key, `null`. Child keys must be unique; when two children share a key, only the first child will be used.

@hlolli: ah ok that’s a different problem, not related to 673 then

@hlolli: so the question is what is the expected behavior if we provide :keyfn

currently we assume you will produce something

and that nil is a valid key value

yes, it would save sanity to allow nil to be a valid key. I guess react only complains about siblings being duplicated.

so nil is a valid key right now

but two components want to use it

but then the question is, why keyfn doesnt associate the assigned key. Yes, which is strange.

if both are siblings lack :keyfn or both have one, it fails, if one is different than other, it works.

@hlolli: it does

@hlolli: because in one case it the key is nil in the other case the key is undefined which React ignores

ok

so if they both have it they both get assigned nil

if they both don’t have it they both get assigned undefined

the later case is 673

ok, so I understand it right that :keyfn :any-value here = :key nil

@hlolli: look at elements in chrome developer console. Every element has a data-reactid. You will see how they are made up with your examples. I know that's not a theoretical answer, but doing that always gets me out of trouble and it seems simple enough to make sure all sibling elements have a different data-reactid. Always proving a function seems like a good idea as well. I know not completely answering your question 🙂

@cjmurphy Thanks, good to know. I always make sure that when creating elements with cljs functions to give :key wherever possible. That way at least I lose all warning, which is nice.

Yeah, directly :key for dom elements and :keyfn for om next components. I think you will be able to see them all in the developer console, even the ones not being displayed because they are duplicates. Not 100% on that thou.

@dnolen: @dominicm: #673 is present in React 15 but not in React 14. Previously setting key to undefined would count as an absent key, but in React 15 keys are explicitly set to undefined

this is what I’ve gathered from looking at the issue a few weeks ago. A few things might have changed meanwhile

@anmonteiro: ok good to know, well in anycase just conditionally adding key should cover us, simple enough to do

Yeah, I think that would be the fix

@dnolen: meanwhile, I’ll try upgrading React to 15.2 to see if this has been eventually fixed/changed upstream

@anmonteiro: cool, yeah would be interested in hearing about that