add doc strings for json module

[dkirchhof]

No description provided.

[dkirchhof]

Short question: Should we create more specific types for the jsonReviver and jsonReplacer?

[zth]

Short question: Should we create more specific types for the jsonReviver and jsonReplacer?

If they can be typed, sure! cc @glennsl who also had similar comments. Here's the original PR this was taken from in rescript-js: bloodyowl/rescript-js#14. Might have some insight.

[glennsl]

cc @glennsl who also had similar comments.

My comment, for the record :) #16 (comment)

[dkirchhof]

cc @glennsl who also had similar comments.

My comment, for the record :) #16 (comment)

Same thoughts. I could change the types. Maybe in another PR?

[zth]

I'm fine with you doing it in this PR if you want. Up to you.

[dkirchhof]

What about this:

reviver:

@val external parseExnWithReviver: (string, (string, 'a) => 'b) => JSON.t = "JSON.parse"

let reviver1 = (key, value) => {
  let jsType = JSON.Classify.classify(value)

  switch jsType {
  | String(v) => String.length(v)
  | _ => value
  }
}

let reviver2 = (key, value) => {
  if key->String.toLowerCase->String.includes("date") {
    value->Date.fromString->Obj.magic
  } else {
    value
  }
}

let jsonString = `{"parseAsDate":"2023-02-26","parseAsString":"2023-02-26","someNumber":42}`

parseExnWithReviver(jsonString, reviver1)->Console.log
parseExnWithReviver(jsonString, reviver2)->Console.log

replacer:

@val external stringifyWithReplacer: (JSON.t, (string, 'a) => string) => string = "JSON.stringify"

let json =
  Dict.fromArray([
    ("foo", JSON.Encode.string("bar")),
    ("hello", JSON.Encode.string("world")),
  ])->JSON.Encode.object

let replacer1 = (key, value) => {
  if key === "foo" {
    String.toUpperCase(value)
  } else if value === "world" {
    value ++ "!"
  } else {
    value
  }
}

stringifyWithReplacer(json, replacer1)->Console.log

Unfortunately not really useful, because the generic types could be everything.

[zth]

@dkirchhof I guess it's as good as it gets. Thoughts @cknitt @glennsl ?

[glennsl]

@val external parseExnWithReviver: (string, (string, 'a) => 'b) => JSON.t = "JSON.parse"

For this to be sound, 'a and 'b both need to be JSON.t. There are certainly use cases for 'b being anything, but then the return value of parse as a whole can't be guaranteed to be JSON.t.

@val external stringifyWithReplacer: (JSON.t, (string, 'a) => string) => string = "JSON.stringify"

This is trickier. The first parameter, key, can be either a string or a number. In most cases that will be fine, as numbers as auto-coerced to string when used with primitive string operations, but there are cases where the behaviour will still differ. For example, the output of JSON.parse("0") and JSON.parse(0) is different.

The second parameter, value, is entirely unsound as-is. With this variant of the function being restricted to JSON.t, the value of this parameter should also be guaranteed to be JSON.t. For a potential strinigifyAny vairant of this, however, it's not entirely clear to me if the value is guaranteed to be JSON.t when this is called. Otherwise the only safe option is an entirely abstract type, that then needs to be classified with the Type.Classify API. But if I'm reading the specification correctly, it should still be JSON.t.

And I believe the return value can also be JSON.t. Returning a string will result in the value being serialized as a JSON string value.

And finally, replacer can also be an array of either strings or numbers. I'd suggest having a separate variant that just accepts array<string>.

tl;dr: taking all this into account I would propose:

@val external parseExnWithReviver: (string, (string, JSON.t) => JSON.t) => JSON.t = "JSON.parse"
@val external stringifyWithReplacer: (JSON.t, (string, JSON.t) => JSON.t) => string = "JSON.stringify"
@val external stringifyWithFilter: (JSON.t, array<string>) => string = "JSON.stringify"
@val external stringifyAnyWithReplacer: ('a, (string, JSON.t) => JSON.t) => string = "JSON.stringify"

[dkirchhof]

@glennsl I would like to agree with you, but thinking about real use cases for these functions. Often I use a reviver like in my example (reviver2). Date strings will be returned as dates. So I don't need any additional codecs or whatever to map these "raw" objects to some kind of domain models.
If the reviver should return JSON.t (makes sense), I can't use this reviver anymore. Or I have to use something like the following jsonEncodeAny function. But then we will have the same unsoundness like before.

external jsonEncodeAny: 'a => JSON.t = "%identity"

let reviver2 = (key: string, value: JSON.t) => {
  if key->String.toLowerCase->String.includes("date") {
    value->JSON.Decode.string->Option.map(Date.fromString)->jsonEncodeAny
  } else {
    value
  }
}

[dkirchhof]

I would be fine with removing parseToAny... and stringifyAny... 🤷‍♂️

[glennsl]

If the reviver should return JSON.t (makes sense), I can't use this reviver anymore. Or I have to use something like the following jsonEncodeAny function. But then we will have the same unsoundness like before.

Here the unsoundness is at least exposed. It's not possible to do this in a sound way anyway, and I think the next best alternative then is to make the unsoundness explicit and have the user make a conscious decision. This does do that.

I would be fine with removing parseToAny... and stringifyAny...

stringifyAny is fine. If given something that is not a JSON value it will raise an exception, but it's not unsafe.

[dkirchhof]

I replaced the reviver and replacer functions and added all variants of stringifyWithFilter.

[aspeddro]

Could you add the ## Exceptions section? Many functions in this module throw errors.

Example:

https://github.com/rescript-association/rescript-core/blob/a976a8ca8ef106172b0479632567cd404583c251/src/Core__List.resi#L72-L87

[dkirchhof]

Should I remove the "throws a JavaScript exception" in the description if a I add the exceptions block?

[zth]

I'm having a look at this very soon. @cknitt you up for taking a look too?

[zth]

@dkirchhof fantastic work! So good to have all of these examples. I even learned a good amount of new things about these APIs just by reviewing 😄 Great work! 👍

I had a few suggestions, that's all. This is definitively mergable imo.

[dkirchhof]

@zth thanks for your review. Great to help you.
I was a bit too fast. Should it be @raises(Exn.t) instead of @raises(Exn.Error)? And should I change the annotation in the res file as well?

[zth]

You know what, let's skip @raises for now, and do it uniformly in #71.

[zth]

Merging! 🎉