RationalJS / future

A Js.Promise alternative for ReasonML
214 stars 15 forks source link

Compilation broken with bs-platform@8.0.3 #53

Closed leeor closed 4 years ago

leeor commented 4 years ago

I have reason-future as a transitive dependency, and it is preventing me from migrating to bs-platform@8 due to this error:

 | FAILED: src/FutureJs.cmj src/FutureJs.cmi /Users/leeor/repos/anchor/node_modules/reason-future/src/FutureJs.bs.js 
 | /Users/leeor/repos/anchor/node_modules/bs-platform/darwin/bsc.exe -color always -bs-package-name reason-future  -bs-package-output commonjs:src -bs-suffix -I src -w a  -o src/FutureJs.cmj src/FutureJs.reast
 | 
 |   We've found a bug for you!
 |   /Users/leeor/repos/anchor/node_modules/reason-future/src/FutureJs.re 18:5-28:8
 |   
 |   16 │ let fromPromise = (promise, errorTransformer) =>
 |   17 │   Future.make(callback =>
 |   18 │     promise
 |   19 │     |> Js.Promise.then_(res =>
 |    . │ ...
 |   27 │          |> Js.Promise.resolve
 |   28 │        )
 |   29 │   );
 |   30 │ 
 |   
 |   This has type:
 |     Js.Promise.t(unit) (defined as Js.Promise.t(unit))
 |   But somewhere wanted:
 |     unit
 |   
 | [9/9] Building src/FutureResult.cmj

|> ignore needs to be added to the promise |> then_(...) chain.

This might have something to do with the fact that .rei files are not published, but when I tried to type the argument to Future.make it did not change anything.

leeor commented 4 years ago

I see that PR #51 was merged 6 days ago, fixing this issue.

Any plans on releasing a new version with the fix?

painedpineapple commented 4 years ago

Temporary Solution. In your package.json:

"reason-future": "https://github.com/RationalJS/future#661accaf"
briangorman commented 4 years ago

@leeor We just released v2.6.0, which should contain the fix