Conversation
- Add `JSOneshotClosure` which provide an ability to invoke a closure at most once. - Remove `JSClosure.init(_ body: @escaping ([JSValue]) -> Void)` overload, which forces users to write result type in closure. - Add JSClosure lifetime test suites
bde6914 to
4c9b876
Compare
|
Time Change: -525.25ms (6%) ✅ Total Time: 8,162ms
ℹ️ View Unchanged
|
MaxDesiatov
left a comment
There was a problem hiding this comment.
Overall this seems legit with a few suggestions in comments.
Co-authored-by: Max Desiatov <max@desiatov.com>
MaxDesiatov
left a comment
There was a problem hiding this comment.
I wasn't sure about that JSClosure.init overload myself when I introduced, but you're right it brings more harm than good. Also, great idea to simplify a lot of things with JSOneshotClosure! 👍
|
@swiftwasm/jskit-team Could anyone review this change? I want two or more approvals for this kind of API change before merging 🙏 |
Co-authored-by: Jed Fox <git@jedfox.com>
j-f1
left a comment
There was a problem hiding this comment.
at a desktop now. LGTM except for these things
Co-authored-by: Jed Fox <git@jedfox.com>
|
@j-f1 I addressed reviewed points. Could you take a look again? |
| timeout = JSTimer(millisecondsDelay: timeoutMilliseconds, isRepeating: false) { | ||
| fatalError("timer should be cancelled") | ||
| } | ||
| timeout = nil |
There was a problem hiding this comment.
Totally happy to punt this to another issue, but since there isn’t necessarily a need to cancel a setTimeout, could there be an API that doesn’t require holding the JSTimer instance? I guess users could just do
JSObject.global.setTimeout(JSOneshotClosure {
// ...
return undefined
}, 1000)There was a problem hiding this comment.
Yes, we can provide such API in this signature for example.
JSTimer.timeout(millisecondsDelay: 100) {
// ...
}This PR is already too large, so let's implement it in another PR 👍
Co-authored-by: Jed Fox <git@jedfox.com>
Motivation
While integrating async/await feature, I found
JSPromisehas too complex API. (e.g. too many overloads, unsafe generic parameters, and lifetime management)So I want to refine the API before async/await integration. As a first step, I changed
JSClosureAPI.Changes
JSOneshotClosurewhich provides an ability to invoke a closure at most once. This will be suitable to use with functions likePromise.thenJSClosure.init(_ body: @escaping ([JSValue]) -> Void)overload, which forces users to write result type in closure.