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

compiles with rescript@next (11-rc4) #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexMoutonNoble
Copy link

No description provided.

@@ -180,7 +180,7 @@ module MutationQueryReducersMap = {
~safeParse,
) =>
Js.Dict.map(
(. mutationQueryReducer) => mutationQueryReducer->MutationQueryReducer.toJs(~safeParse),
(. mutationQueryReducer) => mutationQueryReducer->MutationQueryReducer.toJs(~safeParse, ...),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only place where new syntax is used that will force the minimum rescript version to 11? I'm not necessarily against it, but wonder if we should consider something like the below instead so we don't have to release a major version.

(. mutationQueryReducer) => (. json, options) => mutationQueryReducer->MutationQueryReducer.toJs(~safeParse, json, options),

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah let me see If i can remove it. I was doing this as a demonstration for my team so was trying everything out.

@@ -313,12 +313,13 @@ module Js_ = {
) => Js.Promise.t<FetchResult.Js_.t<'jsData>> = "mutate"

// onClearStore(cb: () => Promise<any>): () => void;
// TODO: named arguments to binding? - AxM
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you want to discuss this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just curious myself, doesnt need your time. ill remove it.

@jeddeloh
Copy link
Owner

Thanks much for this! I made a couple comments. I think we'll have to remember to update package.json to allow rescript 11 as well.

I've sort of been waiting for the official migration documentation to come out before tackling this, but wondered if it would be possible for library authors to simply specify uncurried: false in bsconfig. At any rate, the set of required changes seem pretty minimal so maybe this is better regardless.

@AlexMoutonNoble
Copy link
Author

AlexMoutonNoble commented Oct 12, 2023 via email

@AlexMoutonNoble AlexMoutonNoble force-pushed the rescript-11 branch 2 times, most recently from cca3797 to fcc7192 Compare October 12, 2023 16:37
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