Closed stevenproctor closed 5 years ago
Let me know if there are any updates, or other information, that you may want/need in helping decide if you want to accept this pull request.
Hi Proctor, thanks for your contribution. I'll take a look and let you know asap.
Copied the comments from failover decoder as a starting point. Missed fixing that piece up.
Will fix it in a bit, and push an update.
On Oct 6, 2019, at 16:14, Joan Llenas notifications@github.com wrote:
@joanllenas commented on this pull request.
In README.md:
@@ -394,6 +394,53 @@ treeDecoder.decode({ // Output: Err({error: "<Node
> decoder failed at key 'children' with error: <Node [] | isUndefined> decoder failed because null can't be decoded with any of the provided oneOf decoders"}) +### JsonDecoder.optional + +>
optional<a>(decoder: Decoder<a>): Decoder<a | undefined>
+ +Creates a decoder that returns a default value on failure. You say here returns a default value but the decoder doesn't have a default valuer param like, for instance, the failover decoder. Do we want to add the default value param?— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.
Thought was to be able to handle a null or undefined, but my thought was to return an undefined to reduce the possibility types of values that were returned by"coercing" the null to an undefined return.
If you have a nicer way to handle returning values for an optional type I am open to suggestions.
On Oct 6, 2019, at 16:12, Joan Llenas notifications@github.com wrote:
@joanllenas commented on this pull request.
In src/json-decoder.ts:
@@ -251,6 +251,27 @@ export namespace JsonDecoder { }); }
- /**
- Tries to decode with
decoder
and returnserror
on failure, but allows
- for
undefined
ornull
values to be present at the top level and returns
- an
undefined
if the value wasundefined
ornull
.- *
- @param decoder The actual decoder to use.
- */
- export function optional(
- decoder: Decoder
- ): Decoder<a|undefined> {
- return new Decoder<a|undefined>((json: any) => {
- if (json === undefined) {
- return ok
(undefined); - } else if (json === null) {
- return ok
(undefined); Is there any reason why we'd prefer returning null instead of undefined here? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.
If you have a nicer way to handle returning values for an optional type I am open to suggestions
Although undefined
and null
are often treated as if they had the same semantic meaning, technically they are not the same. While undefined
means that the key doesn't exist or does not have a value assigned, null
means that the key exists and its value is null
, like in it had a value before, but not anymore
and things like that, so coercing might be problematic for some people.
Do see any issues in returning Decoder<a|undefined|null>
instead?
if (json === undefined) {
return ok<undefined>(undefined);
} else if (json === null) {
return ok<null>(null);
}
Added some additional tests, and I remembered why I "coerced" the null
to an undefined
.
If we have a:
type User = {
firstname: string;
lastname: string;
email?: string;
};
and the corresponding decoder:
const userDecoder = JsonDecoder.object<User>(
{
firstname: JsonDecoder.string,
lastname: JsonDecoder.string,
email: JsonDecoder.optional(JsonDecoder.string)
},
'User'
);
if the JsonDecoder.optional
's return type is of <a | undefined | null>
we can't put the possible null
into the optional User.email
field, since the it will only take a <string | undefined>
.
Went ahead and added a test case for that as well, where it does some of the recursive optionality decoding, as well as outlined that usage of the decoder in the README.md.
As an aside, I am loving this library (especially having done some Elm), and have gotten a number of co-workers to complain when they get to the other portions of the code that does the validation of the JSON object that just uses something like a deeply nested Ramda.where
that just gives a true
/false
for the validation response.
Thanks for your work maintaining this!!!!
we can't put the possible
null
into the optionalUser.email
field, since it will only take a<string | undefined>
. Ok, now I get what you mean. Thanks for clarifying and pushing the changes.Thanks for your work maintaining this I'm a happy maintainer. Daniel, the original author of the library, did a really great job. As a side note, I have spent countless hours enjoying your podcast. Thanks!
@stevenproctor could you please add one last conventional-commit commit message? It's what I use to auto-generate the changelog and the semver version number. (I yet have to create a contributors guide)
In order to do that you can, for instance, make a small change to the readme or apply prettier formatting to the .ts and .spec.ts files. Then the commit message would look something like this:
git commit -am "feat(decoders): Added new decoder for optional fields"
Went with the README.md file. Found some text to clean up.
Tried the prettier, but with no prettier config, it came back with a bunch of changes around trying to change the single quotes into double quotes, and didn't want to introduce that much noise into the pull request.
The feature has been published, v0.3.0. Thanks for your contribution!
Thanks again for this package!! ❤️
Currently decoding an optional value entails using the
oneOf
decoder, and passing in the decoder for the type you want decoded, theisUndefined(undefined)
, andisNull(undefined)
to get an optional value on an object.The downside of doing this means that if you have an error on your object, you lose the details of the error because the error becomes
can't be decoded with any of the provided oneOf decoders
instead of continuing the error trace down further down the structure to the next oneOf decoders or the place where the decoding failed.