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’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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aabewhite
Copy link
Contributor

No description provided.

@@ -148,17 +148,10 @@ extension CGFloat: JXConvertible {
}
#endif

#if canImport(Foundation)
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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.

Copy link
Member

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 @inlinables 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 {
Copy link
Contributor Author

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
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Member

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 {
Copy link
Contributor Author

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.

@aehlke
Copy link

aehlke commented Jun 28, 2023

Could this please be merged?

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.

3 participants