[CLJS-1722] Upgrade ExceptionInfo to proper deftype Created: 03/Aug/16 Updated: 18/Jun/17
Unfortunately this does not play well with cljs-devtools. This problem surfaced when I started playing with ExceptionInfo and cljs-devtools v0.8 which newly supports printing deftypes. ExceptionInfo does not contain getBasis, cljs$lang$type, cljs$lang$ctorStr and similar machinery.
My proposed patch implements ExceptionInfo as a proper deftype and does some patch-work to provide backward compatibility. I'm pretty sure we must not break current contract of ExceptionInfo constructor accepting 3 args and synthesizing other fields on the fly in constructor.
This effectively gives us properly working ExceptionInfo deftype with redefined constructor function wrapping deftype's constructor for backwards compatibility.
Note: With working deftype, we can move IPrintWithWriter and toString implementation to the deftype itself.
|Comment by Thomas Heller [ 04/Aug/16 4:25 AM ]|
Why not just add the missing getBasis, cljs$lang$type, cljs$lang$ctorStr bits per set!?
The patch looks like it would mess up advanced compilation although that is just an instinct not something I verified, did you?
|Comment by Antonin Hildebrand [ 04/Aug/16 4:44 AM ]|
I ran clojurescript tests and I assumed they run also against advanced-mode build. During development when my tests were failing I saw error messages about minified names.
This may seem as a hacky solution, but IMO the original code was also a hack. My hack will stay up-to-date with future changes to deftype implementation. I can imagine people would forget to update this part when touching deftype.
btw. there is another patch coming related to discrepancies between deftype and defrecord. That could have been avoided if defrecord shared common implementation with deftype.
|Comment by Thomas Heller [ 04/Aug/16 6:27 AM ]|
Closure is usually very strict about re-defining stuff but I guess my instincts were wrong, the tests should cover advanced.
My issue with this is that deftype is for defining Clojure-specific types. ExceptionInfo is not since it inherits from Error, just like you can't have a superclass in Clojure you can't in CLJS. So IF we were to change deftype in the future we might break things in unexpected ways that just re-use deftype but aren't actually deftype.
Yes, you have to do some house-keeping but you can't enforce the rules of deftype when dealing with inheritance.
Just my 2 cents, it has advantages to re-use deftype too (as you suggested).
|Comment by Antonin Hildebrand [ 04/Aug/16 6:36 AM ]|
Unfortunately I was unable to look up any comments or docs explaining the reasoning why we do that js/Error inheritance there.
My first attempt to "fix" ExceptionInfo was to simply implement it as an ordinary deftype. And that worked just fine (for my tests). Then I tried to re-implement original behaviours on top just to make it 100% compatible.
|Comment by Antonin Hildebrand [ 04/Aug/16 12:47 PM ]|
Just adding a motivational screenshot:
Those yellow warnings are listing which properties are getting copied by gobject/extend call.