Skip to content

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

Merged
merged 11 commits into from
Jan 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/compatibility.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ jobs:
export PATH="$SWIFTENV_ROOT/bin:$PATH"
eval "$(swiftenv init -)"
make bootstrap
cd Example
cd Example/JavaScriptKitExample
swift build --triple wasm32-unknown-wasi
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@ buttonElement.innerText = "Click me!"
let listener = JSClosure { _ in
alert("Swift is running on browser!")
}
buttonElement.onclick = .function(listener)
buttonElement.onclick = .object(listener)

_ = document.body.appendChild(buttonElement)
61 changes: 55 additions & 6 deletions IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,47 @@ try test("Function Call") {
try expectEqual(func6(true, "OK", 2), .string("OK"))
}

let evalClosure = JSObject.global.globalObject1.eval_closure.function!

try test("Closure Lifetime") {
do {
let c1 = JSClosure { arguments in
return arguments[0]
}
try expectEqual(evalClosure(c1, JSValue.number(1.0)), .number(1.0))
c1.release()
}

do {
let c1 = JSClosure { arguments in
return arguments[0]
}
c1.release()
// Call a released closure
_ = try expectThrow(try evalClosure.throws(c1))
}

do {
let c1 = JSClosure { _ in
// JSClosure will be deallocated before `release()`
_ = JSClosure { _ in .undefined }
return .undefined
}
_ = try expectThrow(try evalClosure.throws(c1))
c1.release()
}

do {
let c1 = JSOneshotClosure { _ in
return .boolean(true)
}
try expectEqual(evalClosure(c1), .boolean(true))
// second call will cause `fatalError` that can be caught as a JavaScript exception
_ = try expectThrow(try evalClosure.throws(c1))
// OneshotClosure won't call fatalError even if it's deallocated before `release`
}
}

try test("Host Function Registration") {
// ```js
// global.globalObject1 = {
Expand All @@ -205,7 +246,7 @@ try test("Host Function Registration") {
return .number(1)
}

setJSValue(this: prop_6Ref, name: "host_func_1", value: .function(hostFunc1))
setJSValue(this: prop_6Ref, name: "host_func_1", value: .object(hostFunc1))

let call_host_1 = getJSValue(this: prop_6Ref, name: "call_host_1")
let call_host_1Func = try expectFunction(call_host_1)
Expand All @@ -223,8 +264,8 @@ try test("Host Function Registration") {
}
}

try expectEqual(hostFunc2(3), .number(6))
_ = try expectString(hostFunc2(true))
try expectEqual(evalClosure(hostFunc2, 3), .number(6))
_ = try expectString(evalClosure(hostFunc2, true))
hostFunc2.release()
}

Expand Down Expand Up @@ -336,7 +377,7 @@ try test("ObjectRef Lifetime") {

let identity = JSClosure { $0[0] }
let ref1 = getJSValue(this: .global, name: "globalObject1").object!
let ref2 = identity(ref1).object!
let ref2 = evalClosure(identity, ref1).object!
try expectEqual(ref1.prop_2, .number(2))
try expectEqual(ref2.prop_2, .number(2))
identity.release()
Expand Down Expand Up @@ -449,21 +490,29 @@ try test("Date") {
}

// make the timers global to prevent early deallocation
var timeout: JSTimer?
var timeouts: [JSTimer] = []
var interval: JSTimer?

try test("Timer") {
let start = JSDate().valueOf()
let timeoutMilliseconds = 5.0

var timeout: JSTimer!
timeout = JSTimer(millisecondsDelay: timeoutMilliseconds, isRepeating: false) {
// verify that at least `timeoutMilliseconds` passed since the `timeout` timer started
try! expectEqual(start + timeoutMilliseconds <= JSDate().valueOf(), true)
}
timeouts += [timeout]

timeout = JSTimer(millisecondsDelay: timeoutMilliseconds, isRepeating: false) {
fatalError("timer should be cancelled")
}
timeout = nil
Comment on lines +506 to +509
Copy link
Member

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)

Copy link
Member Author

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 👍


var count = 0.0
let maxCount = 5.0
interval = JSTimer(millisecondsDelay: 5, isRepeating: true) {
// ensure that JSTimer is living
try! expectNotNil(interval)
// verify that at least `timeoutMilliseconds * count` passed since the `timeout`
// timer started
try! expectEqual(start + timeoutMilliseconds * count <= JSDate().valueOf(), true)
Expand Down
3 changes: 3 additions & 0 deletions IntegrationTests/bin/primary-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ global.globalObject1 = {
throw 3.0
},
},
eval_closure: function(fn) {
return fn(arguments[1])
}
};

global.Animal = function (name, age, isCat) {
Expand Down
55 changes: 20 additions & 35 deletions Sources/JavaScriptKit/BasicObjects/JSPromise.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,11 @@ is of actual JavaScript `Error` type, you should use `JSPromise<JSValue, JSValue
This doesn't 100% match the JavaScript API, as `then` overload with two callbacks is not available.
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.

**IMPORTANT**: instances of this class must have the same lifetime as the actual `Promise` object in
the JavaScript environment, because callback handlers will be deallocated when `JSPromise.deinit` is
executed.

If the actual `Promise` object in JavaScript environment lives longer than this `JSPromise`, it may
attempt to call a deallocated `JSClosure`.
*/
public final class JSPromise<Success, Failure>: ConvertibleToJSValue, ConstructibleFromJSValue {
/// The underlying JavaScript `Promise` object.
public let jsObject: JSObject

private var callbacks = [JSClosure]()

/// The underlying JavaScript `Promise` object wrapped as `JSValue`.
public func jsValue() -> JSValue {
.object(jsObject)
Expand Down Expand Up @@ -52,39 +43,37 @@ public final class JSPromise<Success, Failure>: ConvertibleToJSValue, Constructi
/** Schedules the `success` closure to be invoked on sucessful completion of `self`.
*/
public func then(success: @escaping () -> ()) {
let closure = JSClosure { _ in success() }
callbacks.append(closure)
let closure = JSOneshotClosure { _ in
success()
return .undefined
}
_ = jsObject.then!(closure)
}

/** Schedules the `failure` closure to be invoked on either successful or rejected completion of
`self`.
*/
public func finally(successOrFailure: @escaping () -> ()) -> Self {
let closure = JSClosure { _ in
let closure = JSOneshotClosure { _ in
successOrFailure()
return .undefined
}
callbacks.append(closure)
return .init(unsafe: jsObject.finally!(closure).object!)
}

deinit {
callbacks.forEach { $0.release() }
}
}

extension JSPromise where Success == (), Failure == Never {
/** Creates a new `JSPromise` instance from a given `resolver` closure. `resolver` takes
a closure that your code should call to resolve this `JSPromise` instance.
*/
public convenience init(resolver: @escaping (@escaping () -> ()) -> ()) {
let closure = JSClosure { arguments -> () in
let closure = JSOneshotClosure { arguments in
// The arguments are always coming from the `Promise` constructor, so we should be
// safe to assume their type here
resolver { arguments[0].function!() }
return .undefined
}
self.init(unsafe: JSObject.global.Promise.function!.new(closure))
callbacks.append(closure)
}
}

Expand All @@ -93,7 +82,7 @@ extension JSPromise where Failure: ConvertibleToJSValue {
two closure that your code should call to either resolve or reject this `JSPromise` instance.
*/
public convenience init(resolver: @escaping (@escaping (Result<Success, JSError>) -> ()) -> ()) {
let closure = JSClosure { arguments -> () in
let closure = JSOneshotClosure { arguments in
// The arguments are always coming from the `Promise` constructor, so we should be
// safe to assume their type here
let resolve = arguments[0].function!
Expand All @@ -107,9 +96,9 @@ extension JSPromise where Failure: ConvertibleToJSValue {
reject(error.jsValue())
}
}
return .undefined
}
self.init(unsafe: JSObject.global.Promise.function!.new(closure))
callbacks.append(closure)
}
}

Expand All @@ -118,7 +107,7 @@ extension JSPromise where Success: ConvertibleToJSValue, Failure: JSError {
a closure that your code should call to either resolve or reject this `JSPromise` instance.
*/
public convenience init(resolver: @escaping (@escaping (Result<Success, JSError>) -> ()) -> ()) {
let closure = JSClosure { arguments -> () in
let closure = JSOneshotClosure { arguments in
// The arguments are always coming from the `Promise` constructor, so we should be
// safe to assume their type here
let resolve = arguments[0].function!
Expand All @@ -132,9 +121,9 @@ extension JSPromise where Success: ConvertibleToJSValue, Failure: JSError {
reject(error.jsValue())
}
}
return .undefined
}
self.init(unsafe: JSObject.global.Promise.function!.new(closure))
callbacks.append(closure)
}
}

Expand All @@ -146,13 +135,13 @@ extension JSPromise where Success: ConstructibleFromJSValue {
file: StaticString = #file,
line: Int = #line
) {
let closure = JSClosure { arguments -> () in
let closure = JSOneshotClosure { arguments in
guard let result = Success.construct(from: arguments[0]) else {
fatalError("\(file):\(line): failed to unwrap success value for `then` callback")
}
success(result)
return .undefined
}
callbacks.append(closure)
_ = jsObject.then!(closure)
}

Expand All @@ -165,13 +154,12 @@ extension JSPromise where Success: ConstructibleFromJSValue {
file: StaticString = #file,
line: Int = #line
) -> JSPromise<ResultType, Failure> {
let closure = JSClosure { arguments -> JSValue in
let closure = JSOneshotClosure { arguments -> JSValue in
guard let result = Success.construct(from: arguments[0]) else {
fatalError("\(file):\(line): failed to unwrap success value for `then` callback")
}
return success(result).jsValue()
}
callbacks.append(closure)
return .init(unsafe: jsObject.then!(closure).object!)
}

Expand All @@ -184,13 +172,12 @@ extension JSPromise where Success: ConstructibleFromJSValue {
file: StaticString = #file,
line: Int = #line
) -> JSPromise<ResultSuccess, ResultFailure> {
let closure = JSClosure { arguments -> JSValue in
let closure = JSOneshotClosure { arguments -> JSValue in
guard let result = Success.construct(from: arguments[0]) else {
fatalError("\(file):\(line): failed to unwrap success value for `then` callback")
}
return success(result).jsValue()
}
callbacks.append(closure)
return .init(unsafe: jsObject.then!(closure).object!)
}
}
Expand All @@ -205,13 +192,12 @@ extension JSPromise where Failure: ConstructibleFromJSValue {
file: StaticString = #file,
line: Int = #line
) -> JSPromise<ResultSuccess, Never> {
let closure = JSClosure { arguments -> JSValue in
let closure = JSOneshotClosure { arguments -> JSValue in
guard let error = Failure.construct(from: arguments[0]) else {
fatalError("\(file):\(line): failed to unwrap error value for `catch` callback")
}
return failure(error).jsValue()
}
callbacks.append(closure)
return .init(unsafe: jsObject.then!(JSValue.undefined, closure).object!)
}

Expand All @@ -222,13 +208,13 @@ extension JSPromise where Failure: ConstructibleFromJSValue {
file: StaticString = #file,
line: Int = #line
) {
let closure = JSClosure { arguments -> () in
let closure = JSOneshotClosure { arguments in
guard let error = Failure.construct(from: arguments[0]) else {
fatalError("\(file):\(line): failed to unwrap error value for `catch` callback")
}
failure(error)
return .undefined
}
callbacks.append(closure)
_ = jsObject.then!(JSValue.undefined, closure)
}

Expand All @@ -241,13 +227,12 @@ extension JSPromise where Failure: ConstructibleFromJSValue {
file: StaticString = #file,
line: Int = #line
) -> JSPromise<ResultSuccess, ResultFailure> {
let closure = JSClosure { arguments -> JSValue in
let closure = JSOneshotClosure { arguments -> JSValue in
guard let error = Failure.construct(from: arguments[0]) else {
fatalError("\(file):\(line): failed to unwrap error value for `catch` callback")
}
return failure(error).jsValue()
}
callbacks.append(closure)
return .init(unsafe: jsObject.then!(JSValue.undefined, closure).object!)
}
}
Loading