glenjamin / transit-immutable-js

Transit serialisation for Immutable.js
MIT License
250 stars 31 forks source link

Ability to add custom readers and writers #29

Closed ianks closed 7 years ago

ianks commented 8 years ago

I would like to be able to add the ability to transit moment objects in this library (using https://github.com/ianks/moment-transit).

Would you be open to extending the API to allow for customer readers/writers?

glenjamin commented 8 years ago

Yep, that would be great - I'm surprised it hasn't come up before actually!

ianks commented 8 years ago

I suppose the best way would be to accept some new options (readerHandlers and writerHandlers), which would be merged into the base configuration for transit. Thoughts?

glenjamin commented 8 years ago

I think a withHandlers method could match the records API nicely.

Perhaps accepting an array of read/write pairs?

glenjamin commented 7 years ago

As mentioned in #30 - we could also export the readers and writers from this module, allowing a consumer to combine them in any way they like with other codecs.

glenjamin commented 7 years ago

I've started some WIP on this - https://github.com/glenjamin/transit-immutable-js/pull/31

I reckon I'll also add a withExtraHandlers function which takes an array of 3-tuples, [class, readHandler, writeHandler].

The advantage of this approach is most users won't need to care about the internal transit map data-type.

ianks commented 7 years ago

@glenjamin Would this approach work with https://github.com/ianks/moment-transit?

glenjamin commented 7 years ago

Not as-is, looks like they'd need to either expose their handlers or provide a similar extension mechanism to withExtraHandlers

ianks commented 7 years ago

It would be nice to just have a way to pass arbitrary reader and writers a la moment-transit. If that were possible, we could just create a library of common readers and writers and they could be plug and play without any additional knowledge of immutable transit. I don't know how feasible that is though.

glenjamin commented 7 years ago

As far as I'm aware, transit's reader and writer instances don't compose - only the handlers do.

Here you can see where moment-transit wraps it's handlers into a reader and writer: https://github.com/ianks/moment-transit/blob/master/index.js#L22-L34

That's pretty much what this lib does as well at the moment, exposing the handlers as in the PR will allow people to combine them together and create their own reader/writer instances.

ianks commented 7 years ago

Makes sense. I have to say, the transit API is a little... confusing :)

If you could provide an example in the docs that would be incredible! Thanks for your work on this.

glenjamin commented 7 years ago

Yep, I'll be sure to expand the docs to include examples when the PR gets merged.

In the mean time, would you be able to export the read/write handlers from moment transit - I only just realised its your lib!

I can imagine a couple of levels of API will end up being available:

The transit-immutable-js wrapper, which hides the details of transit's API:

var transitImmutable = require("transit-immutable-js");
var moment = require("moment");
var momentTransit = require("moment-transit");

var transit = transitImmutable.withExtraHandlers([
  {class: moment.fn.constructor, write: momentTransit.MomentHandler, tag: "moment", read: moment}
]);

Or the user assembles the transit reader/writer instances themselves:

var transit = require("transit-js");
var transitImmutable = require("transit-immutable-js");
var moment = require("moment");
var momentTransit = require("moment-transit");

var readHandlers = transitImmutable.handlers.read;
readHandlers.moment = moment;
var reader = transit.reader("json", { handlers: readHandlers });

var writeHandlers = transitImmutable.handlers.write;
writeHandlers.set(moment.fn.constructor, momentTransit.MomentHandler);
var writer = transit.writer("json-verbose", { handlers: writeHandlers });
glenjamin commented 7 years ago

@ianks @gusvargas I've updated the PR #31 with docs, tests and a working implementation.

Do you have any feedback on this before I do a release?

glenjamin commented 7 years ago

31 is now released in v0.7.0.