-
-
Notifications
You must be signed in to change notification settings - Fork 57
Major API change on JSClosure #113
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
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
i️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this seems legit with a few suggestions in comments.
Co-authored-by: Max Desiatov <max@desiatov.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol oops accounts are hard
Uh oh!
There was an error while loading. Please reload this page.
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.