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

Nesting a repromise inside a promise is not sound #8

Closed
ncthbrt opened this issue Feb 10, 2018 · 4 comments
Closed

Nesting a repromise inside a promise is not sound #8

ncthbrt opened this issue Feb 10, 2018 · 4 comments
Labels

Comments

@ncthbrt
Copy link
Contributor

ncthbrt commented Feb 10, 2018

This perhaps might not be a use case you want to support, but if so this limitation should be clearly documented. To give a specific example, Js.Promise.resolve(Repromise.resolve(5)) will collapse.

This of course has implications for interop with existing javascript libraries. A function which has signature 'a => Js.Promise.t('a) can not be soundly typed should 'a be of type Repromise.t('b).

This fork of vow is able to solve this case: http://github.com/ncthbrt/vow but is not without the additional overhead of an extra wrapper.

@aantron
Copy link
Owner

aantron commented Feb 12, 2018

Indeed, the discussion about that is here: rescript-lang/rescript-compiler#2345 (comment).

The TL;DR summary is: fixing and not fixing this issue both have negative implications for interop with JavaScript, and for now my guess is that it is better not to fix it, however it is only a guess, so please give any information to the contrary (or in support) that you come across.


More detail:

  • Functions like 'a => Js.Promise.t('a) should be pretty rare, and I think most of them will be promise manipulators (like Promise.resolve, the most obvious function with this type). We don't want people to use these functions from JS anyway, because they are inherently broken. For example, in the case of Promise.resolve, we are replacing it with repromise's Repromise.resolve. That suggests that it is not very important to fix this.
  • Fixing it makes interop between repromise and JS promises non-trivial, as one now has to convert repromises to JS promises before passing them to JS, and wrap promises that come back from JS. So there is a definite penalty to fixing this.

For now, I think it's better to

  1. Push the burden of dealing with functions like 'a => Js.Promise.t('a) to bindings authors. If an author encounters such a function, we should give them tools like runtime checks for whether the argument is a repromise or not, so they can manually make the binding sound.
  2. Ask people to let us know if any of the assumptions above are wrong, in particular if 'a => Js.Promise.t('a) is something that occurs commonly in functions people want to bind, and is something we really need to give first-class support for.

And, of course, all of this should be loudly documented as repromise takes shape :)

I want to also point out that none of the above is any way "final." Repromise is an exploration project, we will learn and adapt as we go.

@ncthbrt
Copy link
Contributor Author

ncthbrt commented Feb 12, 2018

I agree with this sentiment. Just wanted to make it clear that if this is a limitation of repromise, it should be thoroughly documented so that binding authors are aware of the possible pitfalls.

@aantron
Copy link
Owner

aantron commented Jul 17, 2018

@ncthbrt There is now some initial documentation about this in Repromise's new doc site.

@ncthbrt
Copy link
Contributor Author

ncthbrt commented Jul 17, 2018

Awesome. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants