swiftwasm / JavaScriptKit

Swift framework to interact with JavaScript through WebAssembly.
https://swiftpackageindex.com/swiftwasm/JavaScriptKit/main/documentation/javascriptkit
MIT License
670 stars 44 forks source link

Gracefully handle unavailable `JSBridgedClass` #190

Closed MaxDesiatov closed 2 years ago

MaxDesiatov commented 2 years ago

Force unwrapping in class var constructor may crash for classes that are unavailable in the current environment. For example, after https://github.com/swiftwasm/WebAPIKit/pull/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.

github-actions[bot] commented 2 years ago

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 commented 2 years ago

Trying this in https://github.com/swiftwasm/WebAPIKit/pull/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.

j-f1 commented 2 years ago

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

MaxDesiatov commented 2 years ago

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