Fork me on GitHub
#om
<
2016-07-12
>
hlolli12:07:51

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}))
But this will (and vice versa with search-results carrying a :keyfn)
(def search-query (om/factory SearchQuery {:keyfn :search-query}))
(def search-results (om/factory SearchResults))

cjmurphy12:07:56

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.

dominicm12:07:51

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

dnolen12:07:49

@dominicm: they are different actually

cjmurphy12:07:56

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

dnolen12:07:01

build makes instances factory makes a factory fn

dominicm12:07:49

@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?

dnolen12:07:39

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

dnolen12:07:48

build signature is very different

dnolen12:07:11

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

dominicm12:07:16

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

dnolen12:07:13

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

dominicm12:07:30

@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.

hlolli12:07:17

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

dnolen12:07:38

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

hlolli12:07:58

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.

dnolen12:07:22

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

dnolen12:07:25

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

dnolen12:07:35

currently we assume you will produce something

dnolen12:07:44

and that nil is a valid key value

hlolli12:07:06

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

dnolen12:07:24

so nil is a valid key right now

dnolen12:07:31

but two components want to use it

hlolli12:07:44

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

hlolli12:07:47

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

dnolen12:07:12

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

dnolen12:07:03

so if they both have it they both get assigned nil

dnolen12:07:16

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

dnolen12:07:24

the later case is 673

hlolli13:07:49

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

cjmurphy13:07:40

@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 🙂

hlolli13:07:28

@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.

cjmurphy13:07:20

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.

anmonteiro13:07:49

@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

anmonteiro13:07:02

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

dnolen13:07:22

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

anmonteiro13:07:33

Yeah, I think that would be the fix

anmonteiro13:07:50

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

dnolen13:07:13

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