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.

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

Explicitly uncurry rescript-apollo-client #159

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

zatchheems
Copy link

Purpose

To allow pre-Rescript v11 projects to use this library without reconfiguring ppx flags or setting "uncurried": false in one's rescript.json (previously the now deprecated bsconfig.json).

Background

This exists as a subsequent step following an intermediate config-only solution wherein this library temporarily supports the "uncurried": false flag to quickly get this working with Rescript ≥ 11.

Caveats

This required the installation of @rescript/core, a new library that deprecates the old Js library that shipped with Rescript originally. Though this libary is an explicit install now, it will be folded into the Rescript ecosystem and shipped out in later releases.

For now, this was added to avoid a strange error wherein Js.Dict.map relied on a curried implementation.
In contrast, RescriptCore.Dict.mapValues does not.

This will need some testing against the EXAMPLES since it is not working correctly but I've done some preliminary testing against an external project of mine and appears to compile correctly.

@jeddeloh
Copy link
Owner

In general, are we ready to cut a release of the previous stuff so that we're clear for this to be merged in when ready? I have been hesitant due to the requirement of a dev build of graphql-ppx, but open to anyone's thoughts. FWIW, every merge to master publishes a dev package in this repo. You can give the current version at try which is currently 3.2.1-ea2f9b7.0

@jeddeloh
Copy link
Owner

Probably should have @illusionalsagacity in that last comment

@illusionalsagacity
Copy link
Contributor

In general, are we ready to cut a release of the previous stuff so that we're clear for this to be merged in when ready?

I tried out 3.2.1-ea2f9b7.0, a check of some queries & mutations looks good. I didn't have to make any code changes at all for it to compile / run on 10.1.4.

I have been hesitant due to the requirement of a dev build of graphql-ppx, but open to anyone's thoughts.

I've been using 1.2.4-1345e061.0 for some time, but I bumped that version in EXAMPLES and the devDependencies because the peerDependencies for graphql-ppx in the 1.2.3 release is wrong (only allows ^9.1.3 but 10 works fine) and to allow reproduction of that issue. Since I'm already using a dev build of graphql-ppx I can't really say for sure it's not required but I don't think it would be

@illusionalsagacity
Copy link
Contributor

We've been using 4.0.0 for a minute now, haven't noticed any regressions.

I've managed to get a build published of my graphql-ppx pr to @illusionalsagacity/graphql-ppx 2.0.0-f885fc66.0 if anyone needs to use that for testing their own upgrades.

@zatchheems
Copy link
Author

zatchheems commented Jan 3, 2025

We've been using 4.0.0 for a minute now, haven't noticed any regressions.

I've managed to get a build published of my graphql-ppx pr to @illusionalsagacity/graphql-ppx 2.0.0-f885fc66.0 if anyone needs to use that for testing their own upgrades.

@illusionalsagacity I've finally gotten some time to test this out. I've been using your published graphql-ppx fork and am still encountering an annoying uncurrying issue:

FAILED: src/queries/Queries.cmj

  We've found a bug for you!
  /<project dir>/src/Queries.res

  Signature mismatch:
  ...
  Values do not match:
    let parse: Raw.t => t (curried)
  is not included in
    let parse: Raw.t => t (uncurried)
  /<project dir>/node_modules/rescript-apollo-client/src/ApolloClient__Types.res:27:3-23:
    Expected declaration
  /<project dir>/src/Queries.res:
    Actual declaration

not sure if I'm missing anything... other than changing the name of the dependency in my rescript.json to your library and installing it I have made no other changes.

if it's helpful, I've factored out a fragment from my GraphQL schema since I know that's been an issue for graphql-ppx in the past.

@illusionalsagacity
Copy link
Contributor

@illusionalsagacity I've finally gotten some time to test this out. I've been using your publishing graphql-ppx fork and am still encountering an annoying uncurrying issue:

Ahh yeah, did you try adding the "uncurried": true option to the "graphql" options in bsconfig / rescript.json? I had to do that for our app's config, but I didn't remember having to do it for the EXAMPLES project in this repo.

@zatchheems
Copy link
Author

I did try adding "uncurried": true to my rescript.json, yeah. currently trying to get this working with an existing project and not this repo (although I am planning on resolving merge conflicts and continuing work on this PR soon!)

@zatchheems zatchheems force-pushed the main branch 2 times, most recently from d7b27f0 to 41cfb74 Compare January 6, 2025 23:42
@zatchheems zatchheems marked this pull request as ready for review January 8, 2025 00:08
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.

3 participants