Improve bindings

[Mabi19]

I was playing around with the binding implementation and made something useful. This PR allows for two new things:

• chaining .as calls properly, i.e. bind(num).as((v) => v * 2).as((v) => v * 2) will multiply by 4
• deep reactive access via unwrapping Bindings obtained from .as() callbacks, i.e. bind(primaryNetworkKey).as((key) => bind(network, key)).as((net) => bind(net, "icon_name")) will return a string wrapped in a single binding while preserving reactivity.

However, I~~m not~~ originally wasn't sure on a lot of the implementation (which is why this ~~is~~ was a draft PR). Namely:

• This splits out regular bindings and transforming bindings into separate objects. ~~This means that you can't just check that something is a binding with instanceof Binding anymore, so it breaks backwards compatibility.~~
• ~~BindingTransform and DataBinding currently aren't type compatible. This breaks backwards compatibility even more.~~
• ~~BindingTransform currently isn't exported out of the module. This makes the first issue even worse.~~
• ~~This may be remedied by (a) exporting BindingTransform, (b) using a little-known technique called lying by adding a Symbol.hasInstance handler to Binding that also returns true if it's a BindingTransform, or (c) all of the above.~~
• ~~Even more extreme measures would be needed to make BindingTransform type-compatible with Binding. I'm currently thinking making Binding an interface (so that the one returned by bind()'s and the one returned by .as()'s private properties don't need to match)~~ Both implementations now extend a Binding<T> abstract base class, so you don't need to care which one you have (as long as you have a concrete T - only one of the subclasses unwraps nested bindings).
• ~~Deep reactivity is still ugly, and a shorthand for the common case of a constant property key would be really nice. I thought of adding a .prop(key: string) handler to bindings which returns this.as((obj) => bind(obj, key)), but that can't be typed easily as not all bindings will output Connectables.~~ A shorthand has been added
• You mentioned once that you also wanted to make async bindings work; I didn't handle Promises here because it would require an additional layer of indirection, and I don't see the use case for async bindings myself
• ~~I haven't been able to test this in an actual app, mostly because of point 1 (which is now invalid). I'll probably try it later.~~ Both simple test cases and my actual setup have been tested and work fine.
• ~~The docs will need to be updated. I'm still expecting some of the details here to change, so they're left as is right now.~~ The docs have now been updated to reflect the changes.

[Mabi19]

I've done some experimenting and the best I can do for a .prop() function is something like this:

prop<K extends keyof Value>(key: K): Value extends Connectable ? TransformBinding<Value, DataBinding<Value[K]>> : never {
    return this.as((obj: any) => {
        if (typeof obj.connect !== "function") {
            throw new Error("Binding.prop only works on bindings containing Connectables.")
        }
        return bind(obj, key) as DataBinding<Value[K]>
    }) as any;
}

This function would return never if the binding isn't of a Connectable. That's a really janky API, though, so I'm not inclined to include it (yet).

EDIT (like 5 minutes later lol): This works for some reason and I have no idea why!

// look ma, no type assertions!
prop<T extends Connectable, K extends keyof T>(this: Binding<T>, key: K): Binding<T[K]> {
    return this.as((obj) => {
        if (typeof obj.connect !== "function") {
            throw new Error("Binding.prop only works on bindings containing Connectables.")
        }
        return bind(obj, key)
    });
}

Since it only uses Binding and not any of the specific ones, it should be safe to put it in the base class. (.as() is in a similar boat, so I'll probably move it as well)

It errors properly, too!

const a = Variable(123);
// dummy connectable because I was too lazy to make a proper one
const b = Variable({
    data: "abc",
    connect(signal, callback) {
        return 0;
    },
    disconnect(id) {
        
    },
} as Connectable);
bind(a).prop("toString") // error, yay!
bind(b).prop("abc") // the key has intellisense and the result is inferred as `Binding<string>` :D

I will probably implement this ~~tomorrow~~ now.

[Mabi19]

I consider the new bindings to basically be feature-complete by now, so I'm marking this as ready for review. It is still untested in a real app, though, and the docs will need to change a bit (which I want to do at the end, since stuff may still change).

[vafu]

@Mabi19 you might find this interesting https://github.com/Aylur/astal/issues/132#issuecomment-2497052603

[Mabi19]

@Mabi19 you might find this interesting https://github.com/Aylur/astal/issues/132#issuecomment-2497052603

Oh yeah, co-opting RxJS or similar is definitely an option to solve these issues too - though my implementation was born mostly from wanting to hack on the Astal bindings for fun. Besides, having a "native" implementation of deep reactivity is nicer than making wrappers on observables IMO

[vafu]

It's great to have options :)

[Mabi19]

With the docs updated, I consider the implementation done. The only thing that could be implemented now is Promise unwrapping, and I decided against that (for now at least) because it would add a lot of extra complexity to TransformBinding, and complexity bad. Testing still needs to be done, and I'm in the process of trying these changes on my setup right now.

---

However, I did recently have an idea to implement promise unwrapping that would be way simpler. That is, instead of trying to adapt TransformBinding to promises, do the reverse - make promises do everything that regular subscribables do.

The main issue with promises is that you can't .get() their inner value. .then() is basically .subscribe() already - it only needs a little special logic for when the promise is replaced before it's resolved.

I think that tracking promise values externally - for example with a WeakMap<Promise, unknown>, where for every new promise encountered a PROMISE_PENDING symbol is inserted and then replaced with the actual value when the promise resolves. Then the equivalent of .get() on a promise would be getting its value from the map.

[Mabi19]

Initial testing is done, and it seems to work fine after I fixed one minor oopsie along the way. However, for some reason DataBinding<T> is not assignable to Binding<T | undefined> (but Binding<T> is???). I'm gonna investigate this now

[Mabi19]

It appears that this is due to the internal callback of TransformBindings being incompatible: you can't assign a (v: string) => any to a (v: string | undefined) => any because then the object you assigned it to could now call it with an undefined, which the newly assigned function doesn't understand and the world explodes. (Is this why #transformFn is typed as (v: any) => any in the original implementation? @Aylur).

I'm not sure whether making it (v: any) => any (which fixes the type error) is correct here, though, as that error exists for a reason (and it's too late and I'm too tired right now to try to conclusively prove it).

EDIT: I've determined that changing the type of the transform function should be safe, so I've done it.

[Mabi19]

I found and fixed a potential issue where inner subscriptions wouldn't get cleaned up. I don't think this PR can be any more done at this point (but I've thought that like thrice already, so I'll probably prove myself wrong again :P)

[Mabi19]

A month and a half later: I don't think it makes much sense for the bind function to be static on DataBinding, especially since Binding isn't default-exported anymore (and I assume that's going to be kept). I'm not gonna do that without approval, or at least knowing why it was like that, though.

Also, I think that having a helper that takes a T | Binding<T> and outputs Binding<T> by wrapping raw Ts into a dummy binding would be useful for creating custom function widgets that want to take bindings, like in this FAQ entry: https://aylur.github.io/astal/guide/typescript/faq#custom-widgets-with-bindable-properties. The dummy binding could be implemented better as a direct Binding subclass instead of making a dummy subscribable and making a DataBinding to it like in the FAQ entry. I do not know how to name such a helper, though.

[Mabi19]

I've been thinking a little about how to prevent common mistakes with bindings (mostly coercing them to values directly). Well, extra docs on how they work and what you shouldn't do would be one step towards that, as well as adding passive-aggressive doc comments to toString, but adding a [Symbol.toPrimitive] handler that emits a warning when used could be helpful here too.

[Mabi19]

I re-based the branch, added a bunch of doc-comments, and a warning when implicitly converting a Binding to a string. I still think having a dedicated "dummy binding" might be a good idea, but I'm not sure if it's really necessary.

[Aylur]

Would you mind splitting this PR into two? I like these changes, but I haven't yet tested it, I'll get to it soon. In the meantime it would be nice to merge the smaller changes like the doc comments and the [Symbol.toPrimitive] implementation

[Mabi19]

Would you mind splitting this PR into two?

Sure. Don't know when I'll be able to get to that; probably ~~on like Wednesday or something~~ right now. #273

[Mabi19]

While working on the dummy bindings, I realized that it might be a good idea to forgo using the specific binding type in the .as and bind signatures, since it practically means nothing to the user (and forbids optimizations like dummy bindings returning more dummy bindings on .as.

[Mabi19]

I tried to override .as in DummyBinding so that it returns another DummyBinding. However, since .as is also supposed to unwrap one level of binding now, that reactivity is preserved (i.e. DummyBindings aren't deep). I'm not sure if it would be more intuitive to instead fully remove the reactivity, though.

[Aylur]

close this because of https://github.com/Aylur/astal/pull/369

I do plan to have this feature in Gnim though https://github.com/Aylur/gnim/blob/5d2b734be452e2819f3a7313dbb34fa43c23e5d9/src/jsx/state.ts#L183

if anyone feels like implementing this go ahead, its a low prio for now