Updating to Latest Dependencies Rescript version 11, Rescript/React version 12

[GTDev87]

Updated to latest versions

@apollo/client: 3.5.0 -> 3.10.8 @reasonml-community/graphql-ppx: 1.0.0 -> 1.2.3 @rescript/react: 0.11.0 -> 0.12.2 rescript: 10.1.2 -> 11.1.2 graphql: 14.0.0 -> 16.9.0 react: 18.2.0 -> 18.3.1 react-dom: 18.2.0 -> 18.3.1

Breaking change was new rescript uncurried mode, should be backward compatible.

[GTDev87]

I wonder if uncurried: false is required if it is added as a dependency in a project.

[jeddeloh]

Sorry, totally missed notifications on this. This is really all we need? That would be great. That was my initial hope, but haven't looked into it in a while. Have you tried running the examples directory with this?

[zatchheems]

hello! just chiming in to say that I have a project that relies heavily on this library and this is the last dependency preventing us from upgrading to Rescript 11.

as of right now there's just one error related to currying I can't seem to get around even with changes made in this PR:

FAILED: src/ApolloClient__Utils.cmj

  We've found a bug for you!
  /<project directory>/client/node_modules/rescript-apollo-client/src/ApolloClient__Utils.res:11:72-15:3

   9 │ external asJson: 'any => Js.Json.t = "%identity"
  10 │
  11 │ let safeParse: ('jsData => 'data) => Types.safeParse<'data, 'jsData> =
     │ (parse, jsData) =>
  12 │   switch parse(jsData) {
  13 │   | data => Ok(data)
  14 │   | exception Js.Exn.Error(error) => Error({value: jsData->asJson, erro
     │ r: error})
  15 │   }
  16 │
  17 │ let safeParseAndLiftToCommonResultProps: (

  This function expected 1 argument, but got 2

@jeddeloh I know you don't really maintain this library anymore, any thoughts on maintenance or drop-in alternatives? just wondering what the future of this library is/trying to avoid doing too much rework on my project 🙂

[jeddeloh]

@zatchheems Yeah, I'm not really maintaining this anymore, but I'm very happy to help anyone that would like to take a stab at this. With the error you're getting, how sure are you that this isn't just the first error into a cascade of errors until the library has been converted to uncurried? If you're getting an error at all, I assume it means that just simply setting uncurried: false in the bsconfig of the library is not sufficient. @GTDev87 are you having success running this?

@zatchheems have you tried just going through and converting stuff to uncurried? I really doubt it would take all that long.

[zatchheems]

how sure are you that this isn't just the first error into a cascade of errors until the library has been converted to uncurried?

not sure at all! I tested it very briefly so it's likely just the first thread I'll need to pull.

I took a look at https://github.com/jeddeloh/rescript-apollo-client/pull/153 which seems to have addressed much of converting stuff to uncurried (Edit: or at least til that branch was force-pushed and those commits disappeared). I can take a crack at it.

[jeddeloh]

That is unfortunate. If I remember correctly, it was indeed basically done. @AlexMoutonNoble do you know what happened there?

[AlexMouton]

That is unfortunate. If I remember correctly, it was indeed basically done. @AlexMoutonNoble do you know what happened there?

Hi Joel Ahh. I am not very confident in my approach in that branch. I would approach with caution. Belt is discouraged in 10+, and some of the flattening of functions t => x to t, x => may not be useful either. As I remember it, the ppx generation was also troubled so having this library compile didnt get you very close.

Happy to help too as I can, but will be largely out of contact for the next week or more.

[zatchheems]

just to keep this conversation moving, I have been slowly making my way through the necessary updates. I'll create my own PR when that's ready.

one q: I've copied the bsconfig.json to rescript.json and removed the uncurried: false flag from both. this silences the Rescript 11 compiler warning and should theoretically be backwards-compatible. is this a good solution or too much of a kludge just to avoid releasing a new major version?

[jeddeloh]

Awesome! I think we can forget about backwards compatibility at this point and just shoot for a new major version. There's nothing no new features coming in the near future so people can always just stay on an older version if necessary. Also, sorry for the slow reply and heads up that I'm going in for surgery tomorrow so may be a bit slow to respond this week.

[zatchheems]

@jeddeloh one last hurdle before I open a new PR: I've been trying to make sense of uncurrying calls to onClearStore in ApolloClient__Core_ApolloClient.res and am struggling with an error.

earlier in the file, I uncurried the call like so:

  let onClearStore = t => (~cb) => jsClient->Js_.onClearStore(t, ~cb)

a record is passed as the second argument to preserveJsPropsAndContext and I'm not sure what to do here:

  //<user path>/src/@apollo/client/core/ApolloClient__Core_ApolloClient.res:1064:7-18

  1062 ┆ extract,
  1063 ┆ mutate,
  1064 ┆ onClearStore, <--
  1065 ┆ onResetStore,
  1066 ┆ query,

  This function expected 2 arguments, but got 1

this will likely be the same issue for onResetStore as it has a similar function signature. help appreciated!

[illusionalsagacity]

@jeddeloh one last hurdle before I open a new PR: I've been trying to make sense of uncurrying calls to onClearStore in ApolloClient__Core_ApolloClient.res and am struggling with an error.

earlier in the file, I uncurried the call like so:

  let onClearStore = t => (~cb) => jsClient->Js_.onClearStore(t, ~cb)

a record is passed as the second argument to preserveJsPropsAndContext and I'm not sure what to do here:

  //<user path>/src/@apollo/client/core/ApolloClient__Core_ApolloClient.res:1064:7-18

  1062 ┆ extract,
  1063 ┆ mutate,
  1064 ┆ onClearStore, <--
  1065 ┆ onResetStore,
  1066 ┆ query,

  This function expected 2 arguments, but got 1

this will likely be the same issue for onResetStore as it has a similar function signature. help appreciated!

@zatchheems If you haven't fixed this already--it looks like this stems from the trailing unit argument from this binding:

@send
external onClearStore: (t, ~cb: unit => Js.Promise.t<unit>, unit) => unit = "onClearStore"

I am also at a point where this library is one of the last I need to get updated for uncurried, so @zatchheems I'd be happy to help if you want

[illusionalsagacity]

@jeddeloh @zatchheems

I had a feeling that changing out the internals to explict uncurried functions would work: https://github.com/illusionalsagacity/rescript-apollo-client/pull/3

This branch will compile rescript-apollo-client on either "uncurried": true or false. With one large caveat, it seems like the graphql-ppx is not outputting the right signature for Fragment module types right now:

  139 ┆ Js.log2("mutate.update To-Do: ", todo)
  140 ┆ let _unusedRef = writeFragment(
  141 ┆   ~fragment=module(Fragments.TodoItem),
  142 ┆   ~data={
  143 ┆     __typename: todo.__typename,

  Signature mismatch:
  ...
  Values do not match:
    let parse: Raw.t => t (curried)
  is not included in
    let parse: Raw.t => t (uncurried)
  rescript-apollo-client/src/ApolloClient__Types.res:12:3-23:
    Expected declaration
  rescript-apollo-client/EXAMPLES/src/docs/Docs.res:
    Actual declaration

You have to set this ppx-flag for @reasonml-community/graphql-ppx for uncurried support:

"ppx-flags": [["@reasonml-community/graphql-ppx/ppx", "-uncurried"]],

The branch is based on my optional-records branch but I can reverse the order of them to facilitate putting out a minor release for backwards compatibility.

[zatchheems]

@illusionalsagacity at this point it looks like we've duplicated this work—my changes seem very similar except I opted to explicitly uncurry, with one of the advantages being there are no ppx-flag changes necessary:

https://github.com/jeddeloh/rescript-apollo-client/compare/master...zatchheems:rescript-apollo-client:main

still testing it and ironing out the kinks. to be completely honest, I'm not sure which approach is preferable!

[jeddeloh]

Sorry for such a late reply (my recovery did not go so well 😅). I guess I would not be not too concerned about backwards compatibility at this point since we're not really adding any new features that someone on an old codebase wouldn't have access to. Given that, if the work has already been done to explicitly uncurry this library it's preferable to go with that? Thoughts?

[illusionalsagacity]

Sorry for such a late reply (my recovery did not go so well 😅)

Sorry to hear that, hope things are going better now though

I guess I would not be not too concerned about backwards compatibility at this point since we're not really adding any new features that someone on an old codebase wouldn't have access to. Given that, if the work has already been done to explicitly uncurry this library it's preferable to go with that? Thoughts?

My feeling is that it would make it somewhat easier to upgrade with a technically breaking change that still worked with curried mode if there were other libraries that needed updates still. I'm not firmly attached to this so considering the extra work involved with publishing two versions... (one with explicit uncurried syntax and another with turning uncurried: true on) 🤷

[jeddeloh]

My feeling is that it would make it somewhat easier to upgrade with a technically breaking change that still worked with curried mode if there were other libraries that needed updates still.

That's somewhat compelling for the near-term. Are you suggesting we publish one after the other or maintain two versions for a bit?

[illusionalsagacity]

That's somewhat compelling for the near-term. Are you suggesting we publish one after the other or maintain two versions for a bit?

The former yeah. The second publish should be essentially no work as turning on uncurried and running the formatter will print away the (. ) syntax.

The branch I have now compiles for both ReScript 10 and ReScript 11 with uncurried off. I smoke-tested against the app I'm trying to upgrade and I didn't have to make any changes for ReScript 10. Unfortunately I can't verify on ReScript 11 (curried) since I have one dependency that was using the Reason syntax, and when that got moved to the ReScript syntax it also moved to the uncurried syntax exclusively. I did load up EXAMPLES (by merging #158 into that branch) on ReScript 11 curried and it works.

---

However, I'm still getting this graphql-ppx fragment module signature problem both on my branch and on @zatchheems' (after updating @reasonml-community/[email protected] and [email protected])

FAILED: src/hooksUsage/Query_OverlySimple.cmj

  We've found a bug for you!
  /src/github.com/zatchheems/rescript-apollo-client/EXAMPLES/src/hooksUsage/Query_OverlySimple.res

  Signature mismatch:
  ...
  Values do not match:
    let parse: Raw.t => t (curried)
  is not included in
    let parse: Raw.t => t (uncurried)
  /src/github.com/zatchheems/rescript-apollo-client/src/ApolloClient__Types.res:27:3-23:
    Expected declaration
  /src/github.com/zatchheems/rescript-apollo-client/EXAMPLES/src/hooksUsage/Query_OverlySimple.res:
    Actual declaration

[zatchheems]

I was getting a different error while testing an uncurried project and this looks similar. going out of town and leaving my laptop but will be back with fresh eyes next week.

I am all for a temporary uncurried off solution and moving to fully uncurried later. excited to see this moving!

On Wed, Oct 9, 2024, 21:54 illusionalsagacity @.***> wrote:

That's somewhat compelling for the near-term. Are you suggesting we publish one after the other or maintain two versions for a bit?

The former yeah. The second publish should be essentially no work as turning on uncurried and running the formatter will print away the (. ) syntax.

The branch I have now compiles for both ReScript 10 and ReScript 11 with uncurried off. I smoke-tested against the app I'm trying to upgrade and I didn't have to make any changes for ReScript 10. Unfortunately I can't verify on ReScript 11 (curried) since I have one dependency that was using the Reason syntax, and when that got moved to the ReScript syntax it also moved to the uncurried syntax exclusively. I did load up EXAMPLES (by merging #158 https://github.com/jeddeloh/rescript-apollo-client/pull/158 into that branch) on ReScript 11 curried and it works.

However, I'm still getting this graphql-ppx fragment module signature problem both on my branch and on @zatchheems https://github.com/zatchheems'

FAILED: src/hooksUsage/Query_OverlySimple.cmj

We've found a bug for you! /src/github.com/zatchheems/rescript-apollo-client/EXAMPLES/src/hooksUsage/Query_OverlySimple.res

Signature mismatch: ... Values do not match: let parse: Raw.t => t (curried) is not included in let parse: Raw.t => t (uncurried) /src/github.com/zatchheems/rescript-apollo-client/src/ApolloClient__Types.res:27:3-23: Expected declaration /src/github.com/zatchheems/rescript-apollo-client/EXAMPLES/src/hooksUsage/Query_OverlySimple.res: Actual declaration

— Reply to this email directly, view it on GitHub https://github.com/jeddeloh/rescript-apollo-client/pull/156#issuecomment-2403836780, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACA43TPZDQOP6FNSPCVZ6KDZ2XT6XAVCNFSM6AAAAABKYCOF6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBTHAZTMNZYGA . You are receiving this because you were mentioned.Message ID: @.***>

[illusionalsagacity]

The uncurry-internals branch also works when I link @reasonml-community/graphql-ppx against my changes for the ppx

So the branch works with EXAMPLES in the following dependency combinations:

graphql-ppx	rescript	uncurried
1.2.4-1345e061.0	10.x.x	false
1.2.4-1345e061.0	11.1.4	false
PR branch	11.1.4	true

EXAMPLES doesn't seem to have full code coverage of the package though

[jeddeloh]

Awesome. Yeah, sadly, examples only covers pretty basic usage. Do we want to start merging stuff in and publish some pre release packages. I assume we want to merge #155 in first still?

[zatchheems]

I've whipped up a draft PR for the second phase of this upgrade: https://github.com/jeddeloh/rescript-apollo-client/pull/159

note that I'm having trouble getting the EXAMPLES to work. (this is a me problem but I stand by the work I've done thus far!)

[jeddeloh]

@illusionalsagacity So we're just waiting on your ppx changes to land before we can merge this, right?

[jeddeloh]

Or do you want to PR your uncurry-internals branch?

[illusionalsagacity]

Or do you want to PR your uncurry-internals branch?

Ah, that branch is based on the optional records change in #154, it'd probably be easier to review if we merged that and I can squash the actual changes in #160?

[jeddeloh]

Closing in favor of #160