Support pr-str serialization/read-string deserialization

[piranha]

So I'm thinking that would be nice to have, and I can implement it myself (I have it done for DateTime in my project already), but the question is - what's the best way to do it?

What I have right now is:

(extend-type goog.date.DateTime
  IPrintWithWriter
  (-pr-writer [o writer opts]
    (-write writer "#cljs-time/DateTime ")
    (pr-writer (tc/to-long o) writer opts)))

(cljs.reader/register-tag-parser! 'cljs-time/DateTime
  (fn [long]
    (tc/from-long long)))

I serialize it as long (I guess it should be faster), but it could be done as #inst, so that it will be interoperable with server-side Clojure?

Anyway, what's your thoughts?

[andrewmcveigh]

I've just noticed that clj-time has https://github.com/clj-time/clj-time/blob/master/src/clj_time/instant.clj so mirroring that is probably the way to go.

Concretely that would mean serializing to #inst provided that the optional cljs-time.instant namespace has been required.

I don't think there's any benefit to having a custom reader tag.

[piranha]

Yeah, if we serialize to #inst, cljs will parse it to Date object itself. Should it maybe be put in cljs-time.extend, rather than creating a new namespace?

[andrewmcveigh]

Should it maybe be put in cljs-time.extend, rather than creating a new namespace?

That was my first thought, but I think I'd like to keep as much symmetry with the clj-time API as possible. Not 100% sure yet.

[piranha]

The problem with stringifying to #inst is that it's parsed back to native js Date and my code fails somewhere. :-)

[piranha]

Ok, I did this in my code:

(extend-protocol t/DateTimeProtocol
  js/Date
  (year [this] (.getYear this))
  (month [this] (inc (.getMonth this)))
  (day [this] (.getDate this))
  (day-of-week [this] (let [d (.getDay this)] (if (= d 0) 7 d)))
  (hour [this] (.getHours this))
  (minute [this] (.getMinutes this))
  (second [this] (.getSeconds this))
  (milli [this] (.getMilliseconds this))
  (after? [this that] (> (.getTime this) (.getTime that)))
  (before? [this that] (< (.getTime this) (.getTime that)))
  (plus- [this period] ((t/period-fn period) + this))
  (minus- [this period] ((t/period-fn period) - this))
  (first-day-of-the-month- [this]
    (goog.date.DateTime. (.getYear this) (.getMonth this) 1 0 0 0 0))
  (last-day-of-the-month- [this]
    (t/minus-
     (goog.date.DateTime. (.getYear this) (inc (.getMonth this)) 1 0 0 0 0)
     (t/period :days 1))))

is that bad? :)

[andrewmcveigh]

So what I'd probably do is either:

Ensure that all #insts are converted using cljs-time.coerce/from-date at the source. There probably shouldn't be too many places where you're reading #insts. (I dunno, maybe there are good reasons why you are?)

If there are many places, or you just want to ensure that every #inst is parsed to a goog.date.* then consider overriding the default tag readers.

I think extending the default date type as above is probably not a good idea.

[piranha]

Hm, overriding default tag reader sounds more like what I should do, since I have too many #insts. I dump Datascript database and then read it, so I can't add conversion myself.

[piranha]

But anyway, this issue is now pull request with serializing to #inst. :)

[andrewmcveigh]

Yeah, thanks.

I've merged into develop for the moment. A test is failing, not sure why, it could be unrelated.

[andrewmcveigh]

Seems to be passing now, so will merge in with next release.

Thanks!