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

Add a generic JSPromise implementation #62

Merged
merged 8 commits into from
Sep 24, 2020
Merged

Add a generic JSPromise implementation #62

merged 8 commits into from
Sep 24, 2020

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Sep 16, 2020

This provides three overloads each for then and catch, and I'm not sure if that's good for the type-checker performance and usability. I was thinking of naming the promise-returning callback version flatMap, but ultimately decided against it.

then overload with two callbacks is not available, because it's impossible to unify success and failure types from both callbacks in a single returned promise without type erasure. I think users should just chain then and catch in those cases so that type erasure is avoided.

@MaxDesiatov MaxDesiatov added the enhancement New feature or request label Sep 16, 2020
*/
public func then(success: @escaping (Success) -> ()) {
let closure = JSClosure {
success(Success.construct(from: $0[0])!)
Copy link
Member

Choose a reason for hiding this comment

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

If fail to construct, we should throw error or report it with a detailed message. Force unwrapping fatal messages is not quite useful to see the error reason for JavaScriptKit users.

It's impossible to unify success and failure types from both callbacks in a single returned promise
without type erasure. You should chain `then` and `catch` in those cases to avoid type erasure.
*/
public final class JSPromise<Success, Failure>: JSValueConvertible, JSValueConstructible
Copy link
Member

Choose a reason for hiding this comment

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

Please note that this class must have the same lifetime with the actual Promise object in JS environment because callback handlers will be deallocated when JSPromise.deinit.

If the actual Promise object in JS environment lives longer than this JSPromise, it may call deallocated JSClosure.

@MaxDesiatov MaxDesiatov marked this pull request as ready for review September 17, 2020 10:25
@j-f1
Copy link
Member

j-f1 commented Sep 20, 2020

I just had an idea: what if the JSPromise class exposed an ObservableObject? We could have some code on the JS side that would attach itself to the promise and tell Swift to update either the value or error property without having to worry about managing Swift closures.

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Sep 20, 2020

That will require a dependency on OpenCombine unfortunately, and it would be best to keep JavaScriptKit the lowest level library in the stack if we ever hope to achieve #61. What about a separate OpenCombineDOM library that allows any JSPromise to become a publisher, and then exposes fetch and timer publishers too? Yes, this multiplies the number of libraries, but the JS ecosystem is fragmented (read "modular") anyway, and we'd have to reflect that to allow users to mix and match what they intend to use in their apps.

@MaxDesiatov MaxDesiatov merged commit c52ea09 into master Sep 24, 2020
@MaxDesiatov MaxDesiatov deleted the jspromise branch September 24, 2020 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants