New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add serializations #15

Merged
merged 3 commits into from Jun 24, 2016

Conversation

Projects
None yet
2 participants
@boris-marinov
Contributor

boris-marinov commented Jun 6, 2016

This serialize derivation adds a toJSON method to the instance prototype and a static fromJSON function to the ADT constructor.

fromJSON does not use the variant constructors but rather it assumes the listed fields are correct and just copies them.

The methods work recursively: toJSON looks for toJSON method on each value and leaves it as it is if it does not have one. fromJSON acceps as a second argument an object containing ADT definitions that applies them if the types match.

Looking forward to hearing your thoughts on this.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jun 11, 2016

Member

Okay, sorry for taking too long to review this. It looks great 👍

I like the idea of passing a parsers object to .fromJSON. We might want to provide a simpler way for people to construct these objects for the things they use in their applications, but right now I don't really have anything in mind (tracking these globally is easy, but not modular, and doesn't play too well with Node modules' semantics).

Maybe as a separate discussion, do we want to support Transit as a serialisation format? It's a specification for an extensible serialisation format which can use JSON as an encoding.

Member

robotlolita commented Jun 11, 2016

Okay, sorry for taking too long to review this. It looks great 👍

I like the idea of passing a parsers object to .fromJSON. We might want to provide a simpler way for people to construct these objects for the things they use in their applications, but right now I don't really have anything in mind (tracking these globally is easy, but not modular, and doesn't play too well with Node modules' semantics).

Maybe as a separate discussion, do we want to support Transit as a serialisation format? It's a specification for an extensible serialisation format which can use JSON as an encoding.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jun 12, 2016

Contributor

I am glad you like it.

I added the missing checks (weird that that jsverify does not test w/ nulls).

The cool think is that the format is not something fancy. So even this can work:
type.fromJSON(value, require('folktale/data')).

The other think I thought was cool was that by passing the 'contents' of the value you are making an assumption about what types do you expect (that is why I am not automatically adding the root type to the pool). Related to that, what do you think we should do in case a value of unknown type arrives? Currently we just return it as is.

Contributor

boris-marinov commented Jun 12, 2016

I am glad you like it.

I added the missing checks (weird that that jsverify does not test w/ nulls).

The cool think is that the format is not something fancy. So even this can work:
type.fromJSON(value, require('folktale/data')).

The other think I thought was cool was that by passing the 'contents' of the value you are making an assumption about what types do you expect (that is why I am not automatically adding the root type to the pool). Related to that, what do you think we should do in case a value of unknown type arrives? Currently we just return it as is.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jun 16, 2016

Contributor

However in order for the current functionality nicely with the built-ins the 'type' string of the ADT must be the same as the name under which it is exported, so:
const Either = data('folktale:Data.Either', { should be const Either = data('Either', {.

Or maybe I should rework it so it uses the type provided...

Contributor

boris-marinov commented Jun 16, 2016

However in order for the current functionality nicely with the built-ins the 'type' string of the ADT must be the same as the name under which it is exported, so:
const Either = data('folktale:Data.Either', { should be const Either = data('Either', {.

Or maybe I should rework it so it uses the type provided...

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jun 16, 2016

Member

We could have a makeParsers([adt1, adt2, adt3, ...]) function that converts the list of things to a map of them.

Member

robotlolita commented Jun 16, 2016

We could have a makeParsers([adt1, adt2, adt3, ...]) function that converts the list of things to a map of them.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jun 17, 2016

Contributor

TL;DR this will be nice but cannot imagine how exactly it would look.

Yes I like that idea. The only issue I see is where to put this function. It does not make sense for it to be part of the derivation as people shouldn't need to import the derivation function (or know that it exists at all) in order to use from/toJSON.

The funny thing is that the function looks almost like something generic that could be placed in the Object module:
fromArray: Array(a) => ( a => String ) => Map(a)
but not exactly.

Also the fact that you need a helper for the default use case bugs me a little (if we consider the toJSON => fromJSON pipeline as the default use case that is).

Your call, tell me what API would you prefer.

Contributor

boris-marinov commented Jun 17, 2016

TL;DR this will be nice but cannot imagine how exactly it would look.

Yes I like that idea. The only issue I see is where to put this function. It does not make sense for it to be part of the derivation as people shouldn't need to import the derivation function (or know that it exists at all) in order to use from/toJSON.

The funny thing is that the function looks almost like something generic that could be placed in the Object module:
fromArray: Array(a) => ( a => String ) => Map(a)
but not exactly.

Also the fact that you need a helper for the default use case bugs me a little (if we consider the toJSON => fromJSON pipeline as the default use case that is).

Your call, tell me what API would you prefer.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jun 17, 2016

Member

True.

I'm assuming that what you meant by "Or maybe I should rework it so it uses the type provided..." was having an object that provides the parsers look like:

{
  Either: { [typeName]: String, fromJson: Function },
  Maybe: { ... },
  ...
}

Then we can pass require('folktale/data') directly, and we can combine parser providers with the ... object spread, so const parsers = { ...require('folktale/data'), ...require('my-types') }.

This is a much simpler API, and uses built-in language features, which is nice.

Member

robotlolita commented Jun 17, 2016

True.

I'm assuming that what you meant by "Or maybe I should rework it so it uses the type provided..." was having an object that provides the parsers look like:

{
  Either: { [typeName]: String, fromJson: Function },
  Maybe: { ... },
  ...
}

Then we can pass require('folktale/data') directly, and we can combine parser providers with the ... object spread, so const parsers = { ...require('folktale/data'), ...require('my-types') }.

This is a much simpler API, and uses built-in language features, which is nice.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jun 21, 2016

Contributor

OK, I think this is a good compromise:
The fromJSON method rebuilds the dictionary by setting the types of the objects as keys.
This process can be turned off by the keysIndicateType flag.
This is also helpful because, after being performed initially, the rebuilding can be turned off during subsequent recursive calls to fromJSON.

Contributor

boris-marinov commented Jun 21, 2016

OK, I think this is a good compromise:
The fromJSON method rebuilds the dictionary by setting the types of the objects as keys.
This process can be turned off by the keysIndicateType flag.
This is also helpful because, after being performed initially, the rebuilding can be turned off during subsequent recursive calls to fromJSON.

@robotlolita robotlolita merged commit 9a2889d into master Jun 24, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@robotlolita robotlolita deleted the Serialize-derivation branch Sep 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment