-
Notifications
You must be signed in to change notification settings - Fork 9
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
API cleanup and work to make scoped execution easier, logging #13
base: main
Are you sure you want to change the base?
Conversation
@@ -148,17 +148,10 @@ extension CGFloat: JXConvertible { | |||
} | |||
#endif | |||
|
|||
#if canImport(Foundation) |
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 found some places where we were already unconditionally importing all Foundation, so I removed all attempts to conditionalize or limit it.
/// Calls an object as a function. | ||
/// | ||
/// - Parameters: | ||
/// - arguments: The arguments to pass to the function. | ||
/// - this: The object to use as `this`, or `nil` to use the global object as `this`. | ||
/// - Returns: The object that results from calling object as a function | ||
@discardableResult @inlinable public func call(withArguments arguments: [JXValue] = [], this: JXValue? = nil) throws -> JXValue { |
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 had to remove @inlinable from some places because I was calling them from non-@inlinable code. I'd like to just remove all the @inlinable stuff because it's so viral and we don't know if we're getting any benefit from it, or even possibly doing harm.
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 agree – I've never seen much evidence of performance gains from @inlinable in JXKit (although it can be a big boost with other sorts of libraries). Go ahead and nuke any @inlinable
s you see, but maybe time a few runs of JXKitTests before and after to ensure we don't have any egregious regressions.
@@ -1098,6 +1093,51 @@ extension JXValue { | |||
self.init(context: context, valueRef: JSObjectMake(context.contextRef, _class, info)) | |||
} | |||
|
|||
// Await the return of this value as a JavaScript `Promise`. | |||
public func awaitPromise(priority: TaskPriority = .medium) async throws -> JXValue { |
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.
Significant change: I got rid of async eval() calls and switched to this awaitPromise method. eval() variants have multiplied with the introduction of modules and closures and files, and I didn't want yet more async variants of everything. Given that the previous async eval() required you to return a promise, this makes sense to me. Open to other names.
let result = await eval(...).awaitPromise(priority: .low)
@@ -1,4 +1,4 @@ | |||
import struct Foundation.URL | |||
import Foundation |
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.
Lots of internal updates here that don't really matter externally, except to simplify our other code a little.
|
||
public init(strict: Bool = true, moduleRequireEnabled: Bool = true, scriptLoader: JXScriptLoader = Self.defaultScriptLoader) { |
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 discovered that we don't really need to make require() support configurable. If you ever specify a root URL for resource loading, we add require() so that you can load modules from the file system. Otherwise we don't.
public static var defaultLog: (String) -> Void = { print($0) } | ||
|
||
/// The logging function to use for JX log messages. | ||
public var log: (String) -> Void |
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.
Added very simple configurable log function that defaults to 'print', and now any messages we log (and by default our SwiftUI error handling) use this.
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.
How about log: (Any...) -> Void
, which would let us use this for a shim for console.log
as well?
@@ -91,40 +93,103 @@ open class JXContext { | |||
} | |||
|
|||
extension JXContext { |
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 played around with eval() and I settled on this approach. We now have 3 eval() types: eval(), evalClosure(), evalModule(). And each has a variant for a String and a variant for a script resource to load.
The new evalClosure() variant interprets the code as a closure body, returning whatever the code 'return's. Within the code you can use $0, $1, etc for closure arguments, and as you'll see below the evalClosure() func has an optional withArguments: param. This replaces the previous 'withValues' that I added. That had to do all sorts of shenanigans with setting and unsetting $0, $1, etc globals. The closure model doesn't have to do that, plus it has the advantage of giving your code a local scope. So it can access and modify existing globals, but anything it declares is local.
Could this please be merged? |
No description provided.