search

bhauman / devcards

Devcards aims to provide a visual REPL experience for ClojureScript
1.53k stars 113 forks source link

edn rendering can fail when rendering map with unsortable keys #35

Closed darwin closed 9 years ago

darwin commented 9 years ago

Introduced by: https://github.com/bhauman/devcards/pull/31#discussion-diff-37261273R72

Chokes on:

{0 "x", :k v}

During sorting the standard cljs.core.compare is called as (compare :k 0) and keyword's IComparable -compare method throws because the second arguments is not a keyword.

At this point I believe this is an issue in cljs. compare method should be more flexible. But I wanted to mention it here, because there are some other cases when compare could throw.

I would propose to implement our own compare method for edn rendering. Either it should be robust in all cases or it should silently fail to sort if something goes wrong.

bhauman commented 9 years ago

This is a problem. @pkobrien what do you think about rolling back the sorted-map stuff? This is an assumption we can't make.

Also I have been thinking about just using pprint. And dropping the edn rendering. I'm betting that pprint is probably a lot faster and more stable.

Unless we want to grow functionality into edn rendering like editing difference highlighting etc. But if that isn't going to happen pprint is probably a better fit.

darwin commented 9 years ago

@bhauman a side note: I'm rendering maps of different sizes. And sometimes devcards decides to render it as EDN (small maps?) and sometimes as code.

I tried to read sources, but I wasn't able to find the exact place where that decision is made. This part looks like the suspect, but I don't see any problem there: https://github.com/bhauman/devcards/blob/master/src/devcards/core.cljs#L283-L296

I would like to force rendering to always be "as code" in my use-case. Here is the code for reference: https://github.com/darwin/plastic/blob/b9f8e10d02ec6440df277700caa6a6861fd17c3e/cljs/src/devcards/plastic/devcards/util.cljs#L57

bhauman commented 9 years ago

There is no explicit forcing of rendering as code or data in the edn renderer. You may be seeing some flexbox layout effects? Hard to know from here. You certainly are banging on this code more than I have :)

pkobrien commented 9 years ago

I agree this is a problem. I guess I learned something about not assuming that all keys could be sorted. We certainly don't want Devcards to choke while displaying unsortable keys. I think the sorting should be rolled back. I also agree that you probably don't want to be in the business of having to maintain rendering logic so if pprint can be used that would be a good move.

bhauman commented 9 years ago

oh we didn't fix this yet