Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

GTDev87
Copy link
Contributor

@GTDev87 GTDev87 commented Jul 12, 2024

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
Copy link
Contributor Author

GTDev87 commented Jul 12, 2024

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

@jeddeloh
Copy link
Owner

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
Copy link

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
Copy link
Owner

@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
Copy link

zatchheems commented Aug 30, 2024 •
edited
Loading

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 #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
Copy link
Owner

jeddeloh commented Sep 1, 2024

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

@AlexMouton
Copy link

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
Copy link

zatchheems commented Sep 3, 2024 •
edited
Loading

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
Copy link
Owner

jeddeloh commented Sep 5, 2024

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
Copy link

@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
Copy link
Contributor

@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
Copy link
Contributor

@jeddeloh @zatchheems

I had a feeling that changing out the internals to explict uncurried functions would work: illusionalsagacity#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
Copy link

@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:

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
Copy link
Owner

jeddeloh commented Oct 9, 2024

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
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

illusionalsagacity commented Oct 10, 2024 •
edited
Loading

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
Copy link

zatchheems commented Oct 10, 2024 via email

@illusionalsagacity
Copy link
Contributor

illusionalsagacity commented Oct 11, 2024 •
edited
Loading

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
Copy link
Owner

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
Copy link

I've whipped up a draft PR for the second phase of this upgrade: #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!)

@illusionalsagacity illusionalsagacity mentioned this pull request Oct 16, 2024
Merged
@jeddeloh
Copy link
Owner

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

@jeddeloh
Copy link
Owner

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

@illusionalsagacity
Copy link
Contributor

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
Copy link
Owner

Closing in favor of #160

@jeddeloh jeddeloh closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GTDev87 @jeddeloh @zatchheems @AlexMouton @illusionalsagacity