yogthos / json-html

Provide EDN/JSON and get a DOM node with a human representation of the data
MIT License
162 stars 15 forks source link

Maybe bug in cljs/sort-map? #13

Closed mmower closed 7 years ago

mmower commented 8 years ago

Hi.

I think the implementation of sort-map may have a bug. Here's some REPL output:

cljs.user=> es
{#fate.core.Entity{:id #uuid "25419eed-0a5a-4fe8-975a-219062f78152"} {}, #fate.core.Entity{:id #uuid "fb5d4598-f30e-45df-9363-21324c7d81fe"} {}, #fate.core.Entity{:id #uuid "4508f72b-1e3f-4450-8687-65d6d4809d89"} {}, #fate.core.Entity{:id #uuid "83484b0e-757e-4b2a-9289-786fe3d03a04"} {}, #fate.core.Entity{:id #uuid "600525a4-59b8-4cab-a346-87ffc2a71166"} {}}
cljs.user=> (j/sort-map es)
{#fate.core.Entity{:id #uuid "25419eed-0a5a-4fe8-975a-219062f78152"} {}}

It appears the problem is in sorted-map-by with the implementation of str-compare since the call to str is returning [object Object] and they all end up in the same key.

I'm not immediately sure how to solve this one.

yogthos commented 8 years ago

Yeah comparing Js objects is kinda tricky. I haven't really considered it originally to be honest. Looks like uuid compiles to an object, so can't really be compared easily. One workaround could be to preprocess the data in a custom function first. For example, you could prewalk and convert the uuid nodes into something meaningful.

mmower commented 8 years ago

Okay maybe a dumb question but... do we need to sort this stuff?

yogthos commented 8 years ago

The problem is that maps are unordered, and without sorting you can end up with different element order each time data is rendered. This makes it tricky to scan if a particular item changed for example.

mmower commented 8 years ago

Okay I can see how that can be useful. On the other hand having to convert the entire data-structure on each render seems an overhead. Perhaps a compromise is to allow sorting to be configured? I'd compromise on losing it.

yogthos commented 8 years ago

Yeah passing a flag could be an option, or perhaps there's a way to compare js objects more intelligently.

yogthos commented 8 years ago

Maybe using something like (js->clj (.keys obj)) might do it.

yogthos commented 7 years ago

should be fixed by https://github.com/yogthos/json-html/commit/5a8dc57709b2dec8903199276056dafc2ce90f3a