search

glenjamin / transit-immutable-js

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

Records support #9

Closed corbt closed 8 years ago

corbt commented 8 years ago

This PR adds record support as discussed in #7. It's based on the work by @thomasboyt in his fork, with a few modifications:

  1. Nesting Records and other ImmutableJS objects within Records is supported.
  2. Filter has been disabled on Record instances. It isn't a method that ImmutableJS supports on records anyway, and has unexpected behavior. However, a filter predicate applied to a Record instance will successfully filter nested ImmutableJS objects that implement filter such as Map (covered in the tests).
  3. If no Record constructor is passed to the library for a particular Record instance, it will default to encoding the record as a Map (it still will throw an error though if you try to decode a Record without passing the correct constructor).
  4. I've also added documentation to the README.

I think this covers everything @glenjamin asked for to merge a PR; if there's anything else necessary I'm happy to make changes!

glenjamin commented 8 years ago

Excellent work! will dive in and review now.

Let me know if you wont have time to follow up, and I can probably finish myself in that case.

glenjamin commented 8 years ago

Let me know if any of that is unclear, and feel free to disagree! :smile:

corbt commented 8 years ago

Ok think I've covered everything you brought up. I'll open a PR on immutableJS for the recordName thing, but I don't think it's worth waiting on that to get this functionality merged.

glenjamin commented 8 years ago

Nice work on this, that was fast!

corbt commented 8 years ago

Hey, would you mind cutting a new release with this functionality? I need it available so I can make a PR for another module in my dependency chain, thanks.