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

Make uncurried mode opt-out. #6249

Merged
merged 2 commits into from
May 9, 2023
Merged

Make uncurried mode opt-out. #6249

merged 2 commits into from
May 9, 2023

Conversation

cristianoc
Copy link
Collaborator

By default, every project is in uncurried mode, unless "uncurried": false is specified in the project config.

By default, every project is in uncurried mode, unless `"uncurried": false` is specified in the project config.
@cristianoc cristianoc requested a review from cknitt May 9, 2023 01:16
@cristianoc cristianoc added this to the v11.0 milestone May 9, 2023
@cristianoc cristianoc merged commit 382988a into master May 9, 2023
13 checks passed
@cristianoc cristianoc deleted the uncurried_mode_opt_out branch May 9, 2023 14:09
@mvaled
Copy link

mvaled commented Jul 12, 2023

Hi @cristianoc,

I was experimenting with 11.0.0-alpha.3 lately in one of my smaller projects. Since I use curried functions a lot; I added "uncurried": false to my project, I would still get errors from dependencies that are not yet migrated to this new standard.

To make a case, let's use rescript-webapi (https://github.com/mvaled/rescript-webapi/tree/rescript-11), which has some code like:

let createElement = Document.createElement(document)
let createTextNode = Document.createTextNode(document)

that exploit the curried.

So, I added "uncurried": false to its bsconfig.json is not enough because its dependencies won't have it yet:

$ rescript
rescript: [1/6] tests/Webapi/Webapi__ReadableStream__test.ast
rescript: [2/6] examples/fetch_examples.cmj
FAILED: examples/fetch_examples.cmj

  We've found a bug for you!
  /home/manu/src/github.com/rescript/rescript-webapi/examples/fetch_examples.res:6:10-28

  4 │ let _ = {
  5 │   Fetch.fetch("/api/hellos/1")
  6 │   ->then(Fetch.Response.text)
  7 │   ->then(text => print_endline(text)->resolve)
  8 │ }

  This function is a curried function where an uncurried function is expected

rescript: [3/6] tests/Webapi/Webapi__ReadableStream__test.d
rescript: [4/6] tests/Webapi/Webapi__Blob__test.cmj
FAILED: tests/Webapi/Webapi__Blob__test.cmj

  We've found a bug for you!
  /home/manu/src/github.com/rescript/rescript-webapi/tests/Webapi/Webapi__Blob__test.res:6:18-7:74

  4 ┆ blob
  5 ┆ ->arrayBuffer
  6 ┆ ->Promise.then(buffer =>
  7 ┆   buffer->Js.Typed_array.ArrayBuffer.byteLength->Js.log->Promise.resolve
  8 ┆ )
  9 ┆ 

  This expression is expected to have an uncurried function

rescript: [5/6] examples/image_examples.cmj
FAILED: examples/image_examples.cmj

  We've found a bug for you!
  /home/manu/src/github.com/rescript/rescript-webapi/examples/image_examples.res:7:16-25:3

   5 │ 
   6 │ let processImage = url => {
   7 │   Promise.make((resolve, reject) => {
   8 │     // showing the process, this can be done more easily (see below)
   . │ ...
  24 │     body->Element.removeChild(~child=img->HtmlImageElement.asNode)->ign
     │ ore
  25 │   })
  26 │ }
  27 │ 

  This expression is expected to have an uncurried function

rescript: [6/6] tests/Webapi/Webapi__ReadableStream__test.cmj
FAILED: tests/Webapi/Webapi__ReadableStream__test.cmj

  We've found a bug for you!
  /home/manu/src/github.com/rescript/rescript-webapi/tests/Webapi/Webapi__ReadableStream__test.res:9:34-75

   7 ┆ 
   8 ┆ let test_cancel = reader =>
   9 ┆   reader->cancel->Promise.then(() => "cancelled"->Js.log->Promise.resol
     ┆ ve)
  10 ┆ 
  11 ┆ let test_cancelWith = reader =>

  This expression is expected to have an uncurried function

FAILED: cannot make progress due to previous errors.

I this case the library @ryyppy/rescript-promise is being compiled with the uncurried-by-default and that prevents the seamlessly upgrade.

This example is just a showcase. In this particular instance, it's easy to even remove the dependency completely. But this example also shows that other projects would have to wait till rescript-webapi migrate to be able to upgrade.

I guess I'm too late for this party; but you might want to reconsider the default. It might have a major impact in the adoption of Rescript 11.

I certainly want to upgrade. The other features are very compelling. But this one change would prevent me to switch because the dependencies are not ready.

@mvaled
Copy link

mvaled commented Jul 12, 2023

Note: compiling rescript-webpi with 11.0.0-alpha.6 (before this change) works like a charm.

@cristianoc
Copy link
Collaborator Author

Can you confirm:

  1. clean up node modules
  2. set "uncurried": "false" in bsconfig.json in the main project

Then the dependencies are compiled with uncurried mode active?
If true, we will need a self-contained repro, which could be even a project with a single line of code.

@mvaled
Copy link

mvaled commented Jul 12, 2023

Hi @cristianoc,

Thanks for your response. Indeed it works after cleaning up. Sorry, I didn't try that before bringing this up.

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.

None yet

3 participants