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

Improve bindings #127

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Improve bindings #127

wants to merge 9 commits into from

Conversation

Mabi19
Copy link

@Mabi19 Mabi19 commented Nov 23, 2024

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, Im 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.

This allows for chaining `.as` calls and unwraps nested bindings
This allows for maximum backwards compatibility without lying about inheritance.
@Mabi19
Copy link
Author

Mabi19 commented Nov 24, 2024

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

Mabi19 commented Nov 24, 2024

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).

@Mabi19 Mabi19 marked this pull request as ready for review November 24, 2024 20:44
@vafu
Copy link

vafu commented Nov 25, 2024

@Mabi19 you might find this interesting #132 (comment)

@Mabi19
Copy link
Author

Mabi19 commented Nov 25, 2024

@Mabi19 you might find this interesting #132 (comment)

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

vafu commented Nov 25, 2024

It's great to have options :)

I also cleaned up a spelling mistake and  corrected a GTK import
@Mabi19
Copy link
Author

Mabi19 commented Nov 26, 2024

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

Mabi19 commented Nov 26, 2024

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

Mabi19 commented Nov 26, 2024

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.

This also makes it so that inner reactivity is only tracked when something's subscribed
@Mabi19
Copy link
Author

Mabi19 commented Nov 28, 2024

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

Mabi19 commented Jan 14, 2025

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

Mabi19 commented Jan 23, 2025

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 Mabi19 changed the title Revamp binding transforms Improve bindings Jan 23, 2025
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.

2 participants