Multiple views with same description causes silent removal of elements (react list key handling is bad)

[tysonzero]

If I have something like:

view_ fooView ""
view_ barView ""

Then barView will just be silently removed from existence. I'm hoping this is just a bug and not some necessary aspect of the way view_ works.

[fisx]

there should be a warning on the browser console about missing keys.

this issue is discussed here: https://bitbucket.org/wuzzeb/react-flux/issues/23/missing-keys-everywhere. i disagree with wuzzeb's conclusions, but i haven't looked into it further.

[fisx]

I think the solution is to toss the monoid instance for generating DOM elements, and make the writer monad statefully add missing keys. This would be a change affecting almost all of react-hs code, but I think it's worth it, particularly since i18n is another thing monoids aren't very good at.

[tysonzero]

There is nothing inherently wrong with Monoid for this stuff. You can have a <> b modify b to change all keys so that they don't overlap with a, (might have to be a little smart about it to preserve associativity) and then you just have all the elements provide some default key. You could perhaps also treat default keys and user specified keys differently, and not deduplicate the user specified keys. Another option would be to at the very end (if this is possible, haven't looked to thoroughly into how the whole virtual dom is stored in Haskell), go through and add unique keys to every element that needs one.

[fisx]

Ok, noted that there are people in favor of keeping the monoid instance. Perhaps it can be done painlessly.

Also, it's probably a good idea to find out how js people do this. For them I hear it's quite painless.

[tysonzero]

I think if at all possible a decent way would be to look through the virtual DOM at the end, right before react is going to start running things and complaining, and annotating all the nodes. But I am not knowledgable enough about the internals of this library to know how feasible that is.

[fisx]

@tysonzero:

Isn't there risk of people manually setting a key value to be generatedkey### and getting a collision?

yes, manual key collisions are still a risk, but we want people to be allowed to be smarter about the keys then the state monad is, and this is the best we could do so far. the naming of the auto-generated keys could clearly be better documented, even though i think the risk of this particular name being picket aren't that high.

One solution that I think might be worth considering is prepending an underscore to manually specified keys, and having auto generated keys not start with an underscore. Then you could also allow some sort of unsafeKey that doesn't prepend an underscore.

that's a good idea. would you like to implement it? i'll re-open the issue for the time being.

[tysonzero]

I'm actually not sure the best way to implement this, since all properties are treated homogeneously in react-hs.

I guess one option is to have a key function with implementation key k = "key" $= "_" <> k and replace all the associated tutorials and examples to use key instead of $=.

Which I guess is slightly better since it's less ad-hoc and "use key instead of ($=)" is a little cleaner than "avoid having a key of value generatedkey###"

Preferably properties would actually be typed properly so that "style" is only allowed to be a list of CSS rules, and "onclick" must be a callback, and so on.

Perhaps we could only export $= and similar from Internal and put all the necessary functions for defining properties in a type correct way in PropertiesAndEvents. Thus enforcing the whole "use key instead of ($=)" thing.

[tysonzero]

On a side note, why are there records defined on PropertyOrHandler_, since its a sum type they are all just as dangerous as fromJust.

One more thing is that it seems kind of redundant having lots of different fields, one for each data constructor, that all mean the same thing, specifically the name (propertyName, propFromThisName ...) field. Perhaps instead of [PropertyOrHandler] we should use something like Map JSString PropertyOrHandler and remove the name field from all the data constructors. That also would prevent duplicate fields with the same value, which I think (correct me if I'm wrong) is what we want.

If we did the above we would make $= use singleton to return Map JSString PropertyOrHandler and then have them be combined with <> and fold and similar.

[fisx]

Perhaps instead of [PropertyOrHandler] we should use something like Map JSString PropertyOrHandler and remove the name field from all the data constructors.

Not sure about semantics of duplicate fields. But I have no other objections.

[fisx]

For my future self (so I won't get lost in the discussion again): #67 resolved this, but the discussion above raises some minor issues with that.

This issue could be closed, or we can leave it open. @tysonzero, up to you!

[tysonzero]

I'll leave it open for now, as some time later I will probably come back and look into doing some of the talked-about refactorings.