Skip to content

Conversation

@OceanOak
Copy link
Contributor

@OceanOak OceanOak commented Sep 22, 2022

tea_html.res

  • Converted "at" function from bs-json to rescript and used it in tea_html.res.
 let rec at = (key_path, decoder) =>
    switch key_path {
    | list{key} => Json.Decode.field(key, decoder)
    | list{first, ...rest} => Json.Decode.field(first, at(rest, decoder))
    | list{} =>
      \"@@"(
        raise,
        Invalid_argument("Expected key_path to contain at least one element"),
      )
    }
  • Replaced Tea_json.Decoder.string, bool, int, and field with their equivalents in rescript-json-combinators.

tea_http.res

Replaced Tea_json.decodeValue with rescript-json-combinators decode. I think it does the same thing.
Please correct me if I am wrong.

Tea_json.decodeValue

  let decodeValue = (Decoder(decoder), value) =>
    try decoder(value) catch {
    | ParseFail(e) => Error(e)
    | _ => Error("Unknown JSON parsing error")
    }

rescript-json-combinators decode

let decode = (json, decode) =>
  try Ok(decode(. json)) catch {
  | DecodeError(msg) => Error(msg)
  }

Other changes

  • Commented out main_json test file (it tests tea_json ). Should we delete it?

@OceanOak
Copy link
Contributor Author

New decodeString

let decodeString = (decoder, string) =>
    try {
      let value = Js.Json.parseExn(string)
      JsonCombinators.Json.Decode.decode(value,decoder)
    } catch {
    /* | JsException e -> Error ("Given an invalid JSON: " ^ e) */
    | _ => Error("Invalid JSON string")
    }

@OceanOak
Copy link
Contributor Author

New Changes:

  • Switched tea_http.res file using the new object decoder provided by rescript-json-combinators library.

Please correct me if I am using it wrong.

Before:

let decoder = map2(
            (bytes, bytesExpected) => {bytes: bytes, bytesExpected: bytesExpected},
            field("loaded", int),
            field("total", int),
          )

After:

let decoder = Json.Decode.object(field =>{
            bytes: field.required(. "loaded", Json.Decode.int),
            bytesExpected: field.required(. "total", Json.Decode.int),
          })
  • Replaced other functions with their equivalents in rescript-json-combinators

@OceanOak
Copy link
Contributor Author

OceanOak commented Oct 3, 2022

tea_mouse.res

  • Still not sure if it is the right place to use the new object decoder here.

Old

 let position = {
  open Tea_json.Decoder
   map2((x, y) => {x: x, y: y}, field("pageX", int), field("pageY", int))
 }

New

open JsonCombinators
let position = Json.Decode.object(field => {
    x: field.required(. "pageX", Json.Decode.int),
    y: field.required(. "pageY", Json.Decode.int),
  })

New decodeEvent

let decodeEvent = (decoder, value: Web_node.event) =>
  value->Obj.magic->JsonCombinators.Json.decode(decoder)

Couldn't commit these changes, got stuck on this error in test_client_drag.res

image

@pbiggar
Copy link
Member

pbiggar commented Oct 3, 2022

This is looking great, thanks.

Commented out main_json test file (it tests tea_json ). Should we delete it?

If there's no tests for the remaining functions, we should probably add tests for them here.

Couldn't commit these changes, got stuck on this error in test_client_drag.res

I'm not sure from looking at this what looks wrong. I'd suggest breaking up the nested function call and adding type definitions everywhere you can - that usually resolves it for me.

@OceanOak
Copy link
Contributor Author

OceanOak commented Oct 4, 2022

New Changes

  • Fully switched tea_html
  • Fully switched tea_mouse
  • Commented out Tea_json we don't need it anymore. should we delete it ? if yes, I will delete it after solving the last remaining issue.
  • Had to comment out parts of two test files just to be able to commit this. The only issue remaining is:
    the map function in rescript-json-combinators takes uncurried functions and we are using curried functions.
    Example:
         input'(
           list{
             type'("text"),
             on(~key="", "input", Json.Decode.map(targetValue, v => v |> int_of_string |> set_value)),
           },
           list{},
         ),

@pbiggar
Copy link
Member

pbiggar commented Oct 5, 2022

The only issue remaining is:
the map function in rescript-json-combinators takes uncurried functions and we are using curried functions.

I haven't really run into this before. What's the solution?

@OceanOak
Copy link
Contributor Author

OceanOak commented Oct 6, 2022

Still looking for one.
Functions are curried by default in Rescript. I don't know if there is a way to make the functions we're using uncurried. Or if we should make a new map function that accepts curried functions.

@pbiggar
Copy link
Member

pbiggar commented Oct 6, 2022

Yeah, I wasn't clear on this part of rescript.

@OceanOak
Copy link
Contributor Author

OceanOak commented Oct 6, 2022

  1. Found a solution for one of the tests:

Old:

        input'(
          list{
            type'("text"),
            on(~key="", "input", Json.Decode.map(v => v |> int_of_string |> set_value, targetValue)),
          },
          list{},
        ),

New:

        input'(
          list{
            type'("text"),
            on(~key="", "input",((. v) => v |> int_of_string |> set_value)|> Json.Decode.map(targetValue)),
          },
          list{},
        ),

Couldn't do the same for the two remaining tests.

  1. Found a questionable solution for one of the tests
    which is creating a new uncurried function:
    let set_value_uncurried =(. n) => set_value(n)
        button(
          list{on(~key="", "click", Json.Decode.map(clientX, set_value_uncurried))},
          list{text("on \"click\", use clientX value")},
        ),

(Didn't commit the second part, I wasn't sure if it is a right solution)
Thoughts?

@OceanOak
Copy link
Contributor Author

OceanOak commented Oct 6, 2022

I asked about converting a function from curried to uncurried and I got that it is just a matter of creating an anonymous function with one calling convention, calling a function with the other calling convention.
Example of converting from curried to uncurried :
(. v) => f(v)
So now all the tests are successfully passing and this library is fully switched to using rescript-json-combinators.
The only remaining question is :

  • Should we delete tea_json.res and main_json.res since they are no longer used?

@pbiggar
Copy link
Member

pbiggar commented Oct 10, 2022

Should we delete tea_json.res and main_json.res since they are no longer used?

Yes, I think so

@pbiggar
Copy link
Member

pbiggar commented Oct 10, 2022

My plan for merging this is to get the darklang repo using this repo instead of the old one, then to test it out to make sure I understand the implications.

@OceanOak
Copy link
Contributor Author

Do we want to add this to "bsc-flags" to automatically open the library in order to use Json.Encode and Json.Decode directly?
"bsc-flags": [ "-open JsonCombinators" ]

@pbiggar
Copy link
Member

pbiggar commented Oct 11, 2022

Do we want to add this to "bsc-flags" to automatically open the library in order to use Json.Encode and Json.Decode directly? "bsc-flags": [ "-open JsonCombinators" ]

Probably not - I think it would be better to be explicit

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants