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

Upgrade to Rescript 11 #94

Merged
merged 14 commits into from
Apr 26, 2024
Merged

Upgrade to Rescript 11 #94

merged 14 commits into from
Apr 26, 2024

Conversation

mrmurphy
Copy link
Contributor

WIP!

  • Support uncurried mode and ReScript 11
  • Remove esy
  • Move all source to .ml or .res

@mrmurphy mrmurphy marked this pull request as draft April 25, 2024 14:05
@ryb73
Copy link
Member

ryb73 commented Apr 25, 2024

Woah, Murphy! It's funny, I was just thinking the other day that I haven't heard from you in a while. This looks like a much needed upgrade, thanks for working on this!

I've only taken a brief look, but I have a few questions:

  • I see the GitHub build action failed with an error of "dune: not found". Hopefully that's easy to fix?
  • I'm curious why the PPX src moved from Reason syntax to Ocaml? I trust that the changes are ok but unfortunately it makes it hard to diff.
  • Do you have push access to master? You probably should at this point, let me know if you need it!

I haven't worked on Reason stuff in a while, and I trust that the changes you've made here are necessary and correct, so I probably won't give this more than the cursory review that I already have. Thanks again!

Edit: sorry, just noticed the PR is in a draft state so maybe wasn't ready for me to look at yet

@mrmurphy
Copy link
Contributor Author

Hey @ryb73 long time no see! Thank you for looking so quickly 🙇

I figured you hadn't worked on Reason stuff in a while, and I'd be on my own with this so I didn't take much time to explain. Sorry about that!

The context I'm working from is that from my perspective, most of the people who were using Reason before have moved to Rescript, which now focuses on JavaScript only and doesn't market native as a target (I don't even know if native is supported as a target, actually). My company fully adopted ReScript, and we rely heavily on Decco. The ReScript 11 upgrade broke decco with its uncurried by default approach, and so we need to upgrade Decco before we can upgrade Rescript. 🎉

I'm curious why the PPX src moved from Reason syntax to Ocaml?

I moved to Dune and ml syntax to simplify the toolchain and maintenance complexity for Rescript programmers. That way if/when my team members come back to make fixes in the future they don't have to learn the almost-the-same-but-not Reason syntax and tooling in addition to native OCaml. (Also, I'm new to this ecosystem and felt more comfortable just using Dune instead of trying to figure out esy as well)

Do you have push access to master?

I think I do!

just noticed the PR is in a draft state

Yeah, I'm stuck right now on some substantial changes? Higher order decoders are generating a chain of single-argument functions instead of a single function with multiple args:

let o_decode = decoder_a => (v) => Decco.optionFromJson(decoder_a)(v)

I'm not sure if that's because of a change I made, or if it was always working that way, but the tests call the decoder as a 2-arity function:

o_decode(s_decode, None)

So I'm sloshing around in the ml code learning the ast types as I go and trying to figure out how to get to the right place. Any chance you'd be into some real-time pairing?!

@mrmurphy
Copy link
Contributor Author

Oh I forgot to mention the calls that are made also, so I need to figure out how to get what's currently being generated:

let o_decode = decoder_a => (v) => Decco.optionFromJson(decoder_a)(v)

into non-curried versions:

let o_decode = (. decoder_a, v) => Decco.optionFromJson(decoder_a, v)

@mrmurphy
Copy link
Contributor Author

Update, I've made it past that problem, and am cruising through the rest of the changes.

@ryb73
Copy link
Member

ryb73 commented Apr 26, 2024 via email

@mrmurphy
Copy link
Contributor Author

Well, @ryb73, for not being sure what you were doing you sure wrote some damn useful stuff 🤣
We're almost done with the conversion!

@Hossman333
Copy link
Contributor

Thank you Murphy!!!! 😭 ❤️

@mrmurphy mrmurphy marked this pull request as ready for review April 26, 2024 20:31
@mrmurphy mrmurphy merged commit e76b729 into master Apr 26, 2024
1 check passed
@mrmurphy mrmurphy deleted the murphy/upgrade-to-rescript-11 branch April 26, 2024 21:09
@mrmurphy mrmurphy mentioned this pull request Apr 26, 2024
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