justinwoo / purescript-simple-json

A simple Purescript JSON library that uses types automatically
http://purescript-simple-json.readthedocs.io
MIT License
133 stars 45 forks source link

WriteForeign instance that omits empty object keys #27

Closed marcopullo closed 6 years ago

marcopullo commented 6 years ago

Awesome library, was able to use Simple.JSON to delete 100+ lines of new type wrappers for encoding/decoding via generics!

Unfortunately, some APIs enforce a distinction between an object key not being present at all and it being present but with a value of undefined or null. In my use case, Google APIs are failing validation on {optionalParm: undefined} and {optionalParm: null} requests (via the Simple.JSON WriteForeign instances) but work fine with {}.

Would it make sense to add a newtype wrapper instance in this library to skip inserting the key at all for the WriteForeign instance? First thought was this seems like a better default for NullOrUndefined but a distinct newtype wrapper works too.

justinwoo commented 6 years ago

The problem we run into is that we don't currently have a way to avoid writing a key, since that would make for requiring us to use some kind of (Foreign -> Maybe a) interface.

One thing I would recommend if you need to use the actual JS object having no field written is to use delete from purescript-record.

I'm headed out soon, but hopefully doing delete (SProxy :: SProxy "optionalParam") parsedRecord by following some of my examples in this thread will help: https://www.reddit.com/r/purescript/comments/7b5y7q/some_extra_examples_of_simplejson_usage/

Feel free to ask on the Slack #purescript or #purescript-beginners channel, someone should be able to help if you get stuck.

marcopullo commented 6 years ago

Thanks for the pointers! Hmm, that does work, though I’ll need to figure out some more machinery to make it ergonomic in the generic case due to the variety of optional params that are used across different API calls. But this is getting outside the scope of Simple.JSON so will take it elsewhere.

Leaving this open in case it’s still a valid request, but feel free to close otherwise.

justinwoo commented 6 years ago

Yeah, I think it does get a little hairy if you need to handle the various cases. Probably best to make sure that when you need the optional deletion, you've already decided on converting to Foreign.

vyorkin commented 6 years ago

I think the easiest solution could be to add an option for switching between these behaviors. As I understand there are only 3 possible cases:

But I'm not sure if it is a good idea though

justinwoo commented 6 years ago

That wouldn't be consistent with the typeclass method signature and would add more hacks for no good reason. Deleting the field from the output record type as suggested in my earlier comment is what you should do, or remove the field from your row type in your constraints and serialize to/from the inferred concrete row type.

justinwoo commented 6 years ago

Solved in 3.0.0 with Maybe being handled as undefined-leaning undefined | null and Nullable for explicit null.