BREAKING: convert @obj and @deriving(abstract) to use optional record fields

[illusionalsagacity]

Resolves #151

BREAKING: Requires optional record fields support from rescript@^10.0.0

Potentially disruptive (but hopefully not): Drops peer-dependency for bs-platform. It seems pretty likely that most people would be moved to at least rescript@9 (which was just a re-name) or melange by this point.

[illusionalsagacity]

@jeddeloh There are more fields that are typed as option<'a> which I haven't converted yet for the result-style types.

i.e. QueryResult, MutationResult, etc.

let result = Query.use()

switch result {
| {data: {me: {todos}}, _} => Array.joinWith(todos, ", ")
| {data: ?None, _} => "<nothing here>"
}

This feels a bit strange to type I think, but curious as to your thoughts on the matter. There's also the possibility of the field being present vs being explicitly undefined--which seems to be handled the same way today by the compiler but I don't think there's any guarantee of that saying the same.

[jeddeloh]

{data: ?None} seems pretty good to me? If the property really isn't guaranteed to be there, it's probably best to model it as such. At least this way the actual data case is a bit cleaner without the Some()?

[illusionalsagacity]

FYI I checked this out and I didn't have to make any code changes in a consuming application, so while technically breaking it shouldn't be disruptive to anyone.

{data: ?None} seems pretty good to me? If the property really isn't guaranteed to be there, it's probably best to model it as such. At least this way the actual data case is a bit cleaner without the Some()?

Ah I should've used an example that was actually applicable--https://github.com/apollographql/apollo-client/blob/main/src/core/types.ts#L142

data is typed as always being returned--errors is typed as optional which seems dubious to me. I'll have to try it in a vanilla project.

[jeddeloh]

Yeah, that typescript for ApolloQueryResult seems impossible. At some point I know Apollo was planning on the ability to return partial data. Is the T somehow partial and returns an empty object or something when there's no data?

[illusionalsagacity]

I was telling a coworker about this distinction with present-and-undefined vs not-present and when we checked the ReScript output, I found the check does not actually seem to make the distinction--so maybe it is moot, though it seems potentially unsafe with values created from javascript.

type t<'data> = { data: 'data, error?: Error.t }
type example = { foo: string, bar: int }

let myData = { data: { foo: "baz", bar: 1 } }

let log = (data) => switch data {
| {error} => Console.log("there was an error")
| {data} => Console.log("hi data")
}

// Generated by ReScript, PLEASE EDIT WITH CARE


function log(data) {
  if (data.error !== undefined) {
    console.log("there was an error");
  } else {
    console.log("hi data");
  }
}

var myData = {
  data: {
    foo: "baz",
    bar: 1
  }
};

export {
  myData ,
  log ,
}
/* No side effect */

https://rescript-lang.org/try?version=v11.1.4&code=C4TwDgpgBMA8DkATAhsZA+KBeKBvKKaAXFEqsgDRQQBONA9jQPwkCidjAdMFAL4BQoSNQAeyALZgANtBz4AZvXokAzsBoBLAHYBzKgCNkNEtp4D+MnuJAARctjwFyJBUpIAiQwC93BoyQBGPj5+CwgeKXodBwAKQmQASmxMFQB3DWAAYwALJzQ8fgAfPFoGGl5kqABhei0VehlOSJ0Y92Bs2mhU5BUoZC1qDhp3BKK8eIqsTBq6hogmqNbsjTzkEf5eIA

[jeddeloh]

Do you want to stick with a binding that says all the properties are always present? I think that's probably fine. Personally, I always added a wrapper around this library to turn these results into a single Loading | Data | Error variant. Probably not a good idea, but you could add another property that has better ergonomics but still preserve the original response exactly as typed for edge cases.

[illusionalsagacity]

Do you want to stick with a binding that says all the properties are always present? I think that's probably fine.

Yeah that seems more correct, and changing the result types seems pretty disruptive to me; same with changing the useQuery / useMutation arguments from named to optional property record.

Personally, I always added a wrapper around this library to turn these results into a single Loading | Data | Error variant. Probably not a good idea, but you could add another property that has better ergonomics but still preserve the original response exactly as typed for edge cases.

Yeah I preferred that too, this used to be built in and was removed at some point. There's definitely a trade-off between ReScript-specific ergonomics and making the bindings look more like regular JS/TS usage for ease of adoption / ability to make the normal docs more relevant to the bindings.

[jeddeloh]

Yeah that seems more correct, and changing the result types seems pretty disruptive to me; same with changing the useQuery / useMutation arguments from named to optional property record.

Sounds good