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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify JSPromise API #115

Merged
merged 4 commits into from Jan 10, 2021
Merged

Simplify JSPromise API #115

merged 4 commits into from Jan 10, 2021

Conversation

kateinoigakukun
Copy link
Member

@kateinoigakukun kateinoigakukun commented Jan 4, 2021

Changes

Remove generic type parameters from JSPromise

These type parameters are type-safe only when it's produced by JSPromise.then and in other cases, it's always not type-safe. So I removed those type parameters.

Remove then and catch overloads

Those overloads made it difficult to solve compile errors 馃槩

Add Promise.resolve and Promise.reject

@github-actions
Copy link

github-actions bot commented Jan 4, 2021

Time Change: +590ms (6%) 馃攳

Total Time: 8,978.5ms

Test name Duration Change
Serialization/Swift Int to JavaScript 2,882.25ms +191.5ms (6%) 馃攳
Serialization/Swift String to JavaScript 2,945.25ms +208.5ms (7%) 馃攳
Object heap/Increment and decrement RC 2,818ms +190.25ms (6%) 馃攳
鈩癸笍 View Unchanged
Test name Duration Change
Serialization/Write JavaScript number directly 167ms +3.5ms (2%)
Serialization/Write JavaScript string directly 166ms -3.75ms (2%)

performance-action

@kateinoigakukun kateinoigakukun requested a review from a team January 4, 2021 02:26
Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

While it's true that arbitrary user code returning a promise may not be type-safe, we know that most if not all promise-returning Web APIs can only fail with JSError and only resolve to a single type. If a user doesn't know what type to expect they can specify their expected type as JSPromise<JSValue, JSValue>.

While it could make sense to shift the reponsibility for dynamic casts to users, I'm not sure if it's an improvement on balance from the ergonomics perspective. This change would break a lot of things in OpenCombineJS and make Publisher conformance on promise-returning APIs harder to implement. For example, how would DOMKit promise-returning APIs like fetch look in DOMKit if this change is merged? I'm also interested to look what impact this has on async/await APIs in the end?

I don't have a strong opinion here to reject the change outright though. I'm interested to hear opinions on this from other people in the meantime.

@MaxDesiatov MaxDesiatov requested a review from a team January 4, 2021 08:53
@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Jan 4, 2021

My explanation was too short, sorry.

My opinion is that JSPromise should be as primitive as possible. The current JSPromise has two responsibilities as a bridge to the Promise API and as a continuation monad.

Promise API: The responsibility for bridging Promise API is to provide an ability to register asynchronous tasks in the JavaScript event loop.

Continuation monad: The responsibility for continuation monad is to provide map and flatMap.

There are some problems when these two responsibilities are implemented on top of the Promise API.

  1. For example, users usually want to use Promise as a continuation monad, but every time they chain in then (map), they need to interact with JavaScript world.
  2. And also, the constraint of single type resolution is not something that Promise has, but is a convention of the individual JavaScript APIs. Therefore, JSPromise is more strongly constrained than JavaScript's Promise.

The main purpose of my change is to make JSPromise's responsibility only to bridge the Promise API, and to delegate the responsibility as a type-safe continuation monad to another type like Combine.Future.

In this way, the user doesn't need to interact with JavaScript to map. This separation of the two responsibilities results in removing generic type parameters.

For example, how would DOMKit promise-returning APIs like fetch look in DOMKit if this change is merged?

Libraries that provide type-safe APIs, such as DOMKit, should not return JSPromise directly to the user, but convert it to a type that provides type-safe continuation monad like Combine.Future.

I'm also interested to look what impact this has on async/await APIs in the end?

This doesn't directly affect the async/await APIs, it's just a point I noticed while implementing the Concurrency feature.

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. This makes sense 馃憤

@MaxDesiatov MaxDesiatov requested a review from a team January 4, 2021 13:33
a closure that your code should call to either resolve or reject this `JSPromise` instance.
*/
public convenience init(resolver: @escaping (@escaping (Result<Success, JSError>) -> ()) -> ()) {
public convenience init(resolver: @escaping (@escaping (Result<JSValue, JSValue>) -> ()) -> ()) {
Copy link
Member

Choose a reason for hiding this comment

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

Could these be JSValueConvertible so users can do something like resolve(.success("Hello, world!"))?

Suggested change
public convenience init(resolver: @escaping (@escaping (Result<JSValue, JSValue>) -> ()) -> ()) {
public convenience init(resolver: @escaping (@escaping (Result< JSValueConvertible, JSValueConvertible >) -> ()) -> ()) {

Copy link
Member Author

Choose a reason for hiding this comment

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

ConvertibleToJSValue is not conforming to Error, so Result can't have Failure as ConvertibleToJSValue 馃槩

Sources/JavaScriptKit/BasicObjects/JSPromise.swift Outdated Show resolved Hide resolved
Sources/JavaScriptKit/BasicObjects/JSPromise.swift Outdated Show resolved Hide resolved
Sources/JavaScriptKit/BasicObjects/JSPromise.swift Outdated Show resolved Hide resolved
Sources/JavaScriptKit/BasicObjects/JSPromise.swift Outdated Show resolved Hide resolved
Sources/JavaScriptKit/BasicObjects/JSPromise.swift Outdated Show resolved Hide resolved
Comment on lines 37 to 45
public init?(from value: JSValue) {
guard let object = value.object, object.isInstanceOf(Self.constructor) else { return nil }
guard let object = value.object else { return nil }
self.init(from: object)
}

public init?(from object: JSObject) {
guard object.isInstanceOf(Self.constructor) else { return nil }
self.init(unsafelyWrapping: object)
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@MaxDesiatov MaxDesiatov requested review from j-f1 and a team January 6, 2021 10:45
@kateinoigakukun
Copy link
Member Author

@j-f1 Could you take a look again?

Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!

@kateinoigakukun kateinoigakukun merged commit b9984f8 into main Jan 10, 2021
@kateinoigakukun kateinoigakukun deleted the katei/simplify-jspromise branch January 10, 2021 23:08
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