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

Gracefully handle unavailable JSBridgedClass #190

Merged
merged 2 commits into from
May 16, 2022

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented May 4, 2022

Force unwrapping in class var constructor may crash for classes that are unavailable in the current environment. For example, after swiftwasm/WebAPIKit#38 was merged, it led to crashes in getCanvas calls due to these optional casts relying on class var constructor always succeeding:

    public static func construct(from value: JSValue) -> Self? {
        if let canvasRenderingContext2D: CanvasRenderingContext2D = value.fromJSValue() {
            return .canvasRenderingContext2D(canvasRenderingContext2D)
        }
        if let gpuCanvasContext: GPUCanvasContext = value.fromJSValue() {
            return .gpuCanvasContext(gpuCanvasContext)
        }
        if let imageBitmapRenderingContext: ImageBitmapRenderingContext = value.fromJSValue() {
            return .imageBitmapRenderingContext(imageBitmapRenderingContext)
        }
        if let webGL2RenderingContext: WebGL2RenderingContext = value.fromJSValue() {
            return .webGL2RenderingContext(webGL2RenderingContext)
        }
        if let webGLRenderingContext: WebGLRenderingContext = value.fromJSValue() {
            return .webGLRenderingContext(webGLRenderingContext)
        }
        return nil
    }

if let gpuCanvasContext: GPUCanvasContext = value.fromJSValue() branch crashes on browsers that don't have GPUCanvasContext enabled.

As we currently don't have a better way to handle unavailable features, I propose making the result type of static var constructor requirement optional. This means you can still declare classes that are unavailable in the host JS environment. Conditional type casts are also available as they were, they will just always return nil, and initializers for these classes will return nil as well.

@MaxDesiatov MaxDesiatov added the bug Something isn't working label May 4, 2022
@MaxDesiatov MaxDesiatov requested a review from a team May 4, 2022 10:28
in the naming. Parts of the JavaScript `Date` API that are not consistent across browsers and JS
implementations are not exposed in a type-safe manner, you should access the underlying `jsObject`
property if you need those.
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix applied by the formatter.

@@ -94,8 +94,8 @@ private func getObjectValuesLength(_ object: JSObject) -> Int {
return Int(values.length.number!)
}

extension JSValue {
public var array: JSArray? {
public extension JSValue {
Copy link
Contributor Author

@MaxDesiatov MaxDesiatov May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix applied by the formatter, as the rest of similar changes here.

@github-actions
Copy link

github-actions bot commented May 4, 2022

Time Change: +140ms (1%)

Total Time: 13,963ms

Test name Duration Change
Serialization/Write JavaScript string directly 352ms +18ms (5%) 🔍
View Unchanged
Test name Duration Change
Serialization/Write JavaScript number directly 299ms -9ms (3%)
Serialization/Swift Int to JavaScript 4,456ms +23ms (0%)
Serialization/Swift String to JavaScript 4,540ms +87ms (1%)
Object heap/Increment and decrement RC 4,317ms +22ms (0%)

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented May 4, 2022

Trying this in swiftwasm/WebAPIKit#41, looks like there's more work to do. Marking this PR as a draft, but I'm still looking forward to any feedback on this as a general approach.

@@ -1,10 +1,10 @@
/// A wrapper around [the JavaScript Array class](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Array)
/// that exposes its properties in a type-safe and Swifty way.
public class JSArray: JSBridgedClass {
public static let constructor = JSObject.global.Array.function!
public static let constructor = JSObject.global.Array.function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For types like these where the global must be available, how do you feel about declaring constructor as JSFunction!?

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 tried this, but then you have to add an explicit JSFunction? type signature on top, which means you still have to unwrap with ! or ? wherever this constructor is used. So after all, there's no benefit of unwrapping it and then wrapping back again into JSFunction? to satisfy the protocol requirement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I had remembered the Foo! and Foo? types being compatible with each other but it seems that isn’t the case (i.e. you can’t conform to a protocol with a Foo? requirement using a member of type Foo!). So this seems like the best approach. 👍

@j-f1
Copy link
Member

j-f1 commented May 4, 2022

Overall I like this! I had been contemplating adding a Boolean supported class var but this is a much better approach.

@MaxDesiatov MaxDesiatov marked this pull request as ready for review May 12, 2022 17:14
@MaxDesiatov MaxDesiatov requested a review from a team May 12, 2022 17:14
@MaxDesiatov
Copy link
Contributor Author

I've verified that this fully works with WebAPIKit, ready for review.

Copy link
Member

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed this PR

@MaxDesiatov MaxDesiatov merged commit ab3da74 into main May 16, 2022
@MaxDesiatov MaxDesiatov deleted the maxd/optional-constructor branch May 16, 2022 16:50
MaxDesiatov added a commit to swiftwasm/WebAPIKit that referenced this pull request May 21, 2022
Updating the API to make it compatible with swiftwasm/JavaScriptKit#190.

* Gracefully handle unavailable APIs

* Update `Package.resolved`

* Use JSKit 0.15.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants