Refactor bindings, introducing joinBindings #208
Conversation
src/Binding.lua
Outdated
| local disconnect = internalData.changeSignal:subscribe(handler) | ||
| internalData.subscriberCount = internalData.subscriberCount + 1 | ||
| function impl.update(newValue) | ||
| -- This operation doesn't make sense; we can leave it as a no-op |
There was a problem hiding this comment.
Let's throw here to catch weird cases (like using a mapped binding as a ref)
src/Binding.lua
Outdated
| for key, value in pairs(upstreamBindings) do | ||
| if Type.of(value) ~= Type.Binding then | ||
| local message = ( | ||
| "Expected arg #1 to contain only bindings, but key %s had a non-binding value" |
There was a problem hiding this comment.
Should we use %q here, in case the key has spaces or something similarly odd?
src/Binding.lua
Outdated
| end | ||
|
|
||
| return binding, setter | ||
| function impl.update(newValue) | ||
| -- This operation doesn't make sense; we can leave it as a no-op |
There was a problem hiding this comment.
Should throw here too.
src/Binding.lua
Outdated
|
|
||
| local BindingInternalApi = {} | ||
|
|
||
| local BindingPublicMethods = {} |
There was a problem hiding this comment.
Should this be changed to bindingPrototype to be congruent with other classes?
| function Binding.create(initialValue) | ||
| local binding = { | ||
| [Type] = Type.Binding, | ||
| function impl.subscribe(callback) |
There was a problem hiding this comment.
Would it make more sense to only subscribe to upstreamBindings once and create/destroy the upstream connections depending on if there are subscribers or not? The prior implementation does this.
Upstream connections could easily add up, considering bindings mapped from this one will invoke subscribe when subscribed to. A good example of this potentially becoming a problem would be when animating transparency.
There was a problem hiding this comment.
Just got back into working on this!
I don't think it's a big deal to pass subscriptions up through to their parents. At the end of the day, someone is going to have to track the connection, and consolidating that at the top is the simplest approach!
src/Binding.lua
Outdated
| internalData.upstreamDisconnect() | ||
| internalData.upstreamDisconnect = nil | ||
| local impl = { | ||
| disconnectMethods = nil, |
|
PR feedback was addressed and I added a changelog entry and (bad) documentation. I'm not sure what a small sample for how to use |
|
LGTM! Works well in my (rather large) UI codebase making use of |
|
Will merge this first thing Monday to cut 1.1! |
Inspired by #204. 😄
Reworks bindings from the ground up and adds support for joining bindings together.
Checklist before submitting:
CHANGELOG.md