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

Updates for DOMKit #174

Merged
merged 19 commits into from
Apr 4, 2022
Merged

Updates for DOMKit #174

merged 19 commits into from
Apr 4, 2022

Conversation

j-f1
Copy link
Member

@j-f1 j-f1 commented Mar 30, 2022

  • [breaking] The ConvertibleToJSValue and JSBridgedType protocols have changed:

    • To conform to ConvertibleToJSValue, you must now provide a jsValue: JSValue { get } property instead of a jsValue() -> JSValue method.
      • existing code that uses ConvertibleToJSValue will still work, as there is a protocol extension that provides a deprecated jsValue() method.
    • JSBridgedType has dropped its value property requirement in favor of inheriting the jsValue property from ConvertibleToJSValue. To update your conforming types, simply rename value to jsValue.
  • [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
  • Allow people to do object[JSString(...)]!(args...)

    • it was previously only possible to do this with String keys, even though the other property accessors worked with JSStrings too.
  • Lower the Xcode concurrency requirements so they work with the new backporting features

  • Implement JSUInt8ClampedArray (wrapping Uint8ClampedArray)

    • now that I think about it, since Swift defines overflow behaviors for all numbers, this type doesn’t make much sense — the only DOMKit usage is in ImageData, so perhaps it would be better to alias it to JSTypedArray<UInt8>?
    • …but they’re two different classes in JS land so I think it’s useful to have both
  • Allow users to subclass JSObject (removed)

    • this is useful for providing ergonomic access to struct-like objects (i.e. options and API return values)
    • now that I think about it, this whole area is full of pitfalls and may be a bad approach (i.e. fromJSValue won’t work)
      • will try to re-implement my needs in userland and then remove this capability
  • Add a new JSObject(cloning: JSObject) initializer, enabling people to point two JSObject instances at the same object. (removed)

    • 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 (removed)
  • [breaking] rename the JSPromise/value API to JSPromise/get() to make it actually possible to use (reverted)

    • currently, it’s shadowed by JSBridgedType.value
    • the API is inspired by Result/get()
    • an alternative would be to rename the JSBridgedType requirement to jsValue, but this would make autocomplete for that somewhat annoying.
      • a follow-on change from this would be to rename ConvertibleToJSValue/jsValue() to toJSValue() to match ConstructibleFromJSValue’s JSValue/fromJSValue() method.
    • thoughts?

@github-actions
Copy link

github-actions bot commented Mar 30, 2022

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

MaxDesiatov commented Mar 31, 2022

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

j-f1 commented Mar 31, 2022

An update on the JSObject changes (see swiftwasm/WebAPIKit#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
Copy link
Member

kateinoigakukun commented Apr 1, 2022

[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
Copy link
Member Author

j-f1 commented Apr 1, 2022

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:

  • ConstructibleFromJSValue cannot use an initializer because we have to return already-existing instances of e.g. booleans from inside of the JSValue
  • we could have both var jsValue: JSValue and func jsValue() -> JSValue, but Xcode doesn’t autocomplete it properly, and it is confusing IMO

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
Copy link
Member

@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
Copy link
Member Author

j-f1 commented Apr 1, 2022

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

j-f1 commented Apr 1, 2022

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
Copy link
Member

kateinoigakukun commented Apr 3, 2022

@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
Copy link
Member Author

j-f1 commented Apr 3, 2022

Ok, I think I’m done making changes.

@j-f1 j-f1 marked this pull request as ready for review April 3, 2022 20:37
Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Seems legit to me, thanks!

@j-f1 j-f1 merged commit 95d0c4c into main Apr 4, 2022
@j-f1 j-f1 deleted the jed/open-object branch April 4, 2022 15:55
kateinoigakukun added a commit that referenced this pull request Apr 6, 2022
@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Apr 10, 2022

  • [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
Copy link
Contributor

For reference, I'm trying to resolve build issues in TokamakUI/Tokamak#471

@j-f1
Copy link
Member Author

j-f1 commented Apr 10, 2022

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.

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