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

Updates for DOMKit #174

Closed j-f1 closed 2 years ago

j-f1 commented 2 years ago
github-actions[bot] commented 2 years ago

Time Change: -35.5ms (8%) ✅

Total Time: 402.5ms

Test name Duration Change
Serialization/Write JavaScript number directly 197.5ms -20.25ms (10%) 👏
Serialization/Write JavaScript string directly 205ms -15.25ms (7%)

performance-action

MaxDesiatov commented 2 years ago

I like get() consistency with Result. Will need to update a bunch of documentation though, including announcement blog posts, at least mentioning that this change was made in a later JSKit version.

No longer supporting [ConvertibleToJSValue], but making [String] convertible is a good call in my opinion.

Not sure about JSObject subclassing, we need some well-defined use cases.

Aliasing for Uint8ClampedArray sounds good in theory, but what happens when users try to bridge instances to that type? Would we be able to convert them to JSTypedArray<UInt8> nicely?

Add a new JSObject(cloning: JSObject) initializer, enabling people to point two JSObject instances at the same object. this is necessary to construct new DOMKit dictionary objects, as there is currently no valid way to call super in a JSObject subclass, and there is unfortunately no way to “transmute” a JSObject into a subclass after getting it from this requires a new swjs_retain call to keep reference counts correct

Curious to see some example code for this and to hear what @kateinoigakukun thinks about this change.

j-f1 commented 2 years ago

An update on the JSObject changes (see https://github.com/swiftwasm/DOMKit/pull/10): I no longer think the subclassing capability (and therefore the init(cloning:) API) is a good idea, so I will remove that feature from this PR and rewrite DOMKit to use a standard JSBridgedType instead.

kateinoigakukun commented 2 years ago

[breaking] rename the JSPromise/value API to JSPromise/get() to make it actually possible to use

I have some objections to this. Task uses Task.value and also Result is going to use Result.value instead of Result.get(), so I like value for consistency.

[breaking] swap the ConvertibleToJSValue conformance on Array and Dictionary to the : ConvertibleToJSValue case instead of the == ConvertibleToJSValue case.

I don't understand the necessity of this change, could you elaborate on the reason? [String] is already ConvertibleToJSValue without this change. My intention for the conformance requirement was to conform both [X] where X: ConvertibleToJSValue and [ConvertibleToJSValue] to ConvertibleToJSValue. Think the following case.

protocol P0 {}

extension Array: P0 where Element == P0 {}
extension Dictionary: P0 where Value == P0, Key == String {}
// If forcing `Element` or `Value` to be non-existential, it breaks existential cases
// extension Array: P0 where Element: P0 {}
// extension Dictionary: P0 where Value: P0, Key == String {}

func takeP0<X: P0>(_: X) {}

func giveExistentialArray(_ xs: [P0]) {
  takeP0(xs)
}
func giveGenericArray<X: P0>(_ xs: [X]) {
  takeP0(xs)
}

func giveExistentialDictionary(_ xs: [String: P0]) {
  takeP0(xs)
}
func giveGenericArray<X: P0>(_ xs: [String: X]) {
  takeP0(xs)
}

Add a new JSObject(cloning: JSObject) initializer, enabling people to point two JSObject instances at the same object.

Is this feature necessary only if we will take subclassing approach?

Implement JSUInt8ClampedArray (wrapping Uint8ClampedArray)

As far as I understand, Uint8ClampedArray has a setter subscript with Double and getter subscript with UInt8. But such subscript cannot be representable in Swift, so limiting the setter with UInt8 seems reasonable to me.

j-f1 commented 2 years ago

I have some objections to this. Task uses Task.value and also Result is going to use Result.value instead of Result.get(), so I like value for consistency.

Oh cool, I didn’t know about that. We should definitely use value, then. That would necessitate changing the JSBridgedType API and possibly the ConvertibleToJSValue API — how does this look?

public protocol JSBridgedType: JSValueCompatible, CustomStringConvertible {
    // renamed from `value`
    var jsValue: JSValue { get }
    init?(from jsValue: JSValue)
}

public protocol ConvertibleToJSValue {
    // renamed from `jsValue` to avoid an autocomplete conflict
    func toJSValue() -> JSValue
}

Alternatively, we could have a unified API:

public protocol JSBridgedType: JSValueCompatible, CustomStringConvertible {
    // [the jsValue member is inherited from ConvertibleToJSValue]
    init?(from jsValue: JSValue)
}

public protocol ConvertibleToJSValue {
    var jsValue: JSValue { get }
}

Note that:

I don't understand the necessity of this change, could you elaborate on the reason? [String] is already ConvertibleToJSValue without this change.

Unfortunately, it isn’t. While you can call [String]().jsValue(), it isn’t formally part of the protocol. Swift only allows a type (such as Array) to conform to a protocol once, so it must be either [ConvertibleToJSValue] or Array where Element: ConvertibleToJSValue that conforms. You can still call .jsValue() on the other one, but it can’t be used as as a parameter to a function that takes any T: ConvertibleToJSValue, for example. My hope is that anyone who needs to pass a [ConvertibleToJSValue] to a parameter of type ConvertibleToJSValue will be able to call .jsValue() on it, to get back a JSValue which is ConvertibleToJSValue.

… sorry that was kinda badly written but hopefully it gets my point across!

I only made this change because my build of DOMKit was breaking.

Is this feature necessary only if we will take subclassing approach? [referring to JSObject(cloning: JSObject)]

Yep, and I’ve pushed an update to remove it.

As far as I understand, Uint8ClampedArray has a setter subscript with Double and getter subscript with UInt8. But such subscript cannot be representable in Swift, so limiting the setter with UInt8 seems reasonable to me.

👍

kateinoigakukun commented 2 years ago

@j-f1

public protocol JSBridgedType: JSValueCompatible, CustomStringConvertible {
    // renamed from `value`
    var jsValue: JSValue { get }
    init?(from jsValue: JSValue)
}

public protocol ConvertibleToJSValue {
    // renamed from `jsValue` to avoid an autocomplete conflict
    func toJSValue() -> JSValue
}

It looks like fine to me 👍

Unfortunately, it isn’t. While you can call [String]().jsValue(), it isn’t formally part of the protocol.

Yes, [String] is not ConvertibleToJSValue but it's implicitly convertible to ConvertibleToJSValue by type-coercing because Array and Dictionary are special container types that are "covariant". Could you tell me the real breaking case in DOMKit?

j-f1 commented 2 years ago

Could you tell me the real breaking case in DOMKit?

The issue was that there’s a property wrapper for JS object keys:

@propertyWrapper public struct ReadWriteAttribute<Wrapped: JSValueCompatible> {
  // ...
}

class Foo: JSValueCompatible {
    // ...
    @ReadWriteAttribute var bar: [String]
}

This did not compile, because [String] did not conform to JSValueCompatible.

j-f1 commented 2 years ago

It looks like fine to me 👍

Do you have a preference for it over the second proposal? I feel like the second one is more ergonomic.

kateinoigakukun commented 2 years ago

@j-f1

This did not compile, because [String] did not conform to JSValueCompatible.

Ah, I got it. Makes sense to me to change the conformance condition.

Do you have a preference for it over the second proposal? I feel like the second one is more ergonomic.

I prefer the second one.

j-f1 commented 2 years ago

Ok, I think I’m done making changes.

MaxDesiatov commented 2 years ago
  • [breaking] The ConvertibleToJSValue conformance on Array and Dictionary has been swapped from the == ConvertibleToJSValue case to the : ConvertibleToJSValue case.
    • This means that e.g. [String] is now ConvertibleToJSValue, but [ConvertibleToJSValue] no longer conforms
    • the jsValue() method still works in both cases
    • to adapt existing code, use one of these approaches:
    • use generics where possible (for single-type arrays)
    • call .map { $0.jsValue() } (or mapValues) to get an array/dictionary of JSValue which you can then use as ConvertibleToJSValue
    • add .jsValue to the end of all of the values in the array/dictionary literal

I'm actually finding this leading to quite hard to diagnose type checker issues, especially in Tokamak. Could this decision be reconsidered? WDYT an alternative could be?

MaxDesiatov commented 2 years ago

For reference, I'm trying to resolve build issues in https://github.com/TokamakUI/Tokamak/pull/471

j-f1 commented 2 years ago

It's definitely up for reconsideration. One option would be I guess to make special property wrappers for arrays/dictionaries of JSValueCompatible things on the DOMKit side. Otherwise there isn’t an alternative that I can think of.