JSONable not usable with types/records in idomatic fashion

[cemerick]

Starting with 18a1b4a68da6ee830c993f64b4a26c353328ab1b, typical usage of deftype and defrecord to define class-level method impls for the JSONable protocol (example) stopped working.

(The workaround is one must use extend-protocol or extend-type instead of an inline JSONable implementation, but that's contrary to user expectations, and absent examining cheshire's source and understanding protocol implementation details, there's no way to know why a type ostensibly satisfying a protocol isn't being json'd as expected.)

#57 raised this and proposed using satisfies? instead, but that's equivalent to reverting the 18a1b4a change, and thus negated its performance benefit.

I see a couple of options:

1. Roll back to satisfies? / find-protocol-impl, sacrificing the performance gain, but recovering expected/typical protocol behaviour.
2. Document JSONable that it needs to be used as it does.
3. Maybe, maybe making JSONable the center of cheshire.generate instead of an auxiliary extension mechanism would be a win all around, with the parts of it that absolutely require cond-based dispatch being poured into a default Object extension of JSONable.

[dakrone]

@cemerick hmm... I see that this is an issue.. I'm not sure yet what the best thing to do in this situation is, I'll think about this.

[cemerick]

Sure. The workaround is easy enough, and JSONable isn't actually documented at all AFAICT?, so not a big hurry, etc.

Another option (call it (4) I guess) I thought of earlier: eliminate JSONable entirely, and provide a special fn code can call into to register a JSON generator for a given class that gets pushed into a threadsafe map or something. This is all generate is using the protocol for anyway, and would accurately calibrate user expectations.

[dakrone]

JSONable isn't actually documented at all AFAICT?

Yeah, right now only add-encoder and remove-encoder are documented, as I think it's good to have a small entry point for this. How feasible do you think it would be to require the use of those methods? Is it worth saying that using defrecord is not supported?

[cemerick]

So, I didn't even notice the add/remove-encoder functions after all this time. This is essentially what I was gesturing towards in my option (4) above; what would make that complete would be to eliminate the protocol entirely, since cheshire isn't benefitting from it at all. Just a threadsafe map of class -> fn would satisfy the existing semantics of how JSONable impls are looked up, and would eliminate any potential confusion caused by the protocol sitting there to begin with.

I still think option (3) might be better all around, but the work necessary to implement and validate it (esp w.r.t. performance regressions) is obviously way larger than just dropping the protocol.

[dakrone]

I still think option (3) might be better all around, but the work necessary to implement and validate it (esp w.r.t. performance regressions) is obviously way larger than just dropping the protocol.

Last time I attempted this, it was indeed a performance regression, which was too bad, it would certainly have simplified the code.

It may be worth another stab at though, since Clojure has changed since the last time I tried it.