-
Notifications
You must be signed in to change notification settings - Fork 429
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
Add initialisers to create an Action from an input property #22
Conversation
return action.errors | ||
} | ||
|
||
public func apply(_ input: Void = ()) -> SignalProducer<Output, ActionError<Error>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially tried to use func apply() -> SignalProducer
here, but Swift complained about this not matching the signature of apply
as defined in ActionProtocol
. The workaround was to at least provide a default value of ()
, but this still feels like a bug to me.
Did you try defining it in an extension on |
I did think about defining it on |
Yeah, I think adding an initializer to |
Ok, so I tried moving it to an FWIW, I don't mind the idea of making Also, just off the top of my head, you could conceivably introduce another implementation that enables concurrent execution of the inner signal producers, or one that cancels an executing producer when executed (i.e., the "merge" and "latest" flatten strategies). Just some ideas! |
Try to make |
I did try that at one point, but it's not valid to define a However ultimately the error isn't that you can't define the error, it's Swift choosing the wrong initialiser at the call site. I think it's to do with the way |
I'd like to play around with it to understand it better and see if I can find a way to make it work in an extension. But that may mean it takes me a little longer to get to this PR. |
1341fdb: To meet the requirement of |
9a6f01d
to
fb9a775
Compare
@ikesyo Of course! I accidentally deleted the variant where Well, now that it works as expected, I've squashed those changes into the original commit. For now we can achieve this purely with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. ✨ Just needs one documentation fix.
} | ||
} | ||
|
||
/// Initializes an action that uses an optional property for its input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one isn't optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Also fixed up some weird tab vs. space problems Xcode inserted for me.
fb9a775
to
54f8c07
Compare
/// current value of `input`. | ||
public init<P: PropertyProtocol, T>(input: P, _ execute: @escaping (T) -> SignalProducer<Output, Error>) where P.Value == T? { | ||
self.init(enabledIf: input.map { $0 != nil }) { | ||
execute(input.value!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this more, I don't think this is safe.
The property's value could change between the enabled check and the time execute is called. If the value has switched back to nil
, then this will assert.
I think this will require some changes to apply
and maybe a different initializer. I don't see how we can make the current approach work. (But I'd be happy to be wrong.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about 07ea7e7? It synchronises checking for isEnabled
and calling executeClosure
, so I think should solve this particular problem.
It feels like there could be a more fundamental refactoring of Action that would remove the need for force-unwrapping (like you were hinting at), but this is the smallest change I could think of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually having trouble reasoning about whether changes to isExecuting
and userEnabled
were truly atomic, so I resolved that by unifying the two properties in a single struct that can be modified atomically via MutableProperty
. See 25a3024.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still working through whether it's safe (trying to verify that it won't deadlock), but it's definitely easier to understand the code here now. 👍🏻
private static func shouldBeEnabled(userEnabled: Bool, executing: Bool) -> Bool { | ||
return userEnabled && !executing | ||
} | ||
private let _state: MutableProperty<ActionState> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our style has been to not prefix private variables with an underscore unless there's a public variable with the same name.
self._state.modify { state in | ||
if state.isEnabled { | ||
state.isExecuting = true | ||
executedProducer = self.executeClosure(input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can return
this from the modify
directly instead of assigning to a var
.
/// current value of `input`. | ||
public init<P: PropertyProtocol, T>(input: P, _ execute: @escaping (T) -> SignalProducer<Output, Error>) where P.Value == T? { | ||
self.init(enabledIf: input.map { $0 != nil }) { | ||
execute(input.value!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still working through whether it's safe (trying to verify that it won't deadlock), but it's definitely easier to understand the code here now. 👍🏻
self._state.modify { state in | ||
if state.isEnabled { | ||
state.isExecuting = true | ||
executedProducer = self.executeClosure(input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel good about calling the executeClosure
inside a lock. I think we need to find a different solution. 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let hasStartedExecution: Bool = self.state.modify { ... }
should do. The executeClosure
need not be moved inside the critical section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we started on this is so that this:
public init<P: PropertyProtocol, T>(input: P, _ execute: @escaping (T) -> SignalProducer<Output, Error>) where P.Value == T? {
self.init(enabledIf: input.map { $0 != nil }) {
execute(input.value!)
}
}
Could guarantee that input.value
was non-nil. I actually don't think the code as written solves even this.
/// enabled. | ||
/// - execute: A closure that returns the signal producer returned by | ||
/// calling `apply(Input)` on the action. | ||
init<P: PropertyProtocol>(enabledIf property: P, _ execute: @escaping (Input) -> SignalProducer<Output, Error>) where P.Value == Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we changed this initializer to this:
init<P: PropertyProtocol>(enabledIf property: P, _ execute: @escaping (Input) -> SignalProducer<Output, ActionError<Error>>) where P.Value == Bool
Then the execute closure could bail if required. This seems like it'd be a nice safety addition in general—for cases when you would need make guarantees.
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried making this change (having the execute closure return SignalProducer<Output, ActionError<Error>>
), but I couldn't figure out a nice way to make the API work without breaking the existing use case.
However, I tried making the return value an optional signal producer, which seems to work nicely: c8791b7.
There's something still wrong to me about this though. I think if we want to truly prove this is safe, we need to make checking the enabled state and grabbing the input value an atomic operation, which means moving the input property into ActionState
somehow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the idea of making ActionState
more powerful by allowing it to carry an optional value, but I can't think of a way to achieve that without leaking implementation details out via ActionProtocol
.
That is, I'd like to make a private initialiser, but because ActionProtocol.init(input:_:)
is in a protocol extension it has to call a public initialiser. Starting to wonder whether the only way to make this safe & atomic is to reintroduce PropertyAction
, so it can redefine isEnabled
in terms of input
while still providing atomic access to the current value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was think that this:
private let executeClosure: (Input) -> SignalProducer<Output, ActionError<Error>>
// new init that uses ActionError
public init<P: PropertyProtocol>(enabledIf property: P, _ execute: @escaping (Input) -> SignalProducer<Output, ActionError<Error>>) where P.Value == Bool { … }
// existing init
public init<P: PropertyProtocol>(enabledIf property: P, _ execute: @escaping (Input) -> SignalProducer<Output, Error>) where P.Value == Bool {
self.init(enabledIf: property) { return execute.promoteErrors(ActionError.ProducerError) }
}
would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the only difference between the two initialisers is the return type of execute
: this makes trailing closures with inferred return types ambiguous, and I don't think forcing users to always explicitly annotate the return type is a good idea.
20523bc
to
73daeba
Compare
73daeba
to
efa93c6
Compare
@mdiep I managed to work out a way to introduce an unambiguous overload, so that we can safely unwrap the optional. However I couldn't work out a way to do so without leaking implementation details, so I'd appreciate your input on the solution I came up with. IMO, given the current limitations of extensions of generic types and the need to use protocol extensions, I think it would ultimately be a cleaner API today to implement this as Having said that, I think this version of the API would work out just fine in practice, so I ultimately don't mind which way we go. And I'm prepared to admit there might be an even cleaner way to refactor this without leaking implementation details! |
Concrete same-type requirement would land in 3.1 though. If these can be converted without breaking the source, IMO it is okay to tolerate an exposed private API for a while, just like how stdlib does with all the underscores. |
This is still on my list! Sorry for the delay. I'll try to get to it this week. |
e247f54
to
fea3dc6
Compare
I think I've managed to make the input property thread-safe with fea3dc6! Related discussion: #88 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I have made a patch to a possible streamlined implementation that does away with the state struct. Feel free to take any parts you like. 😃
4c9a18b
/// - execute: A closure that returns the signal producer returned by | ||
/// calling `apply(Input)` on the action, optionally using | ||
/// the current value of `state`. | ||
init<State: PropertyProtocol>(_internalState property: State, enabledIf isEnabled: @escaping (State.Value) -> Bool, _ execute: @escaping (State.Value, Input) -> SignalProducer<Output, Error>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! The underscore label of the designated initializer can be stripped now, can't it? Like how in #88 Matt's proposed API takes any arbitrary isEnabled
predicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. 👍🏻 Needs a few changes, but nothing major IMO.
public init<P: PropertyProtocol>(enabledIf property: P, _ execute: @escaping (Input) -> SignalProducer<Output, Error>) where P.Value == Bool { | ||
/// calling `apply(Input)` on the action, optionally using | ||
/// the current value of `state`. | ||
public init<State: PropertyProtocol>(_internalState property: State, enabledIf isEnabled: @escaping (State.Value) -> Bool, _ execute: @escaping (State.Value, Input) -> SignalProducer<Output, Error>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should get rid of the _
, rename it to something like state
, and accept that this is going to be a public initializer.
private let userEnabled: (Any) -> Bool | ||
|
||
init(isExecuting: Bool, value: Any, isEnabled: @escaping (Any) -> Bool) { | ||
self.isExecuting = isExecuting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could probably default to false
instead of getting passed in.
/// - input: An Optional property whose current value is used as input | ||
/// whenever the action is executed. The action is disabled | ||
/// whenever the value is nil. | ||
/// - execute: A closure to return a new SignalProducer based on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nil
, Optional
, and SignalProducer
should all be backtick-quoted here.
/// Whether the action should be enabled for the given combination of user | ||
/// enabledness and executing status. | ||
fileprivate var isEnabled: Bool { | ||
return userEnabled(value) && !isExecuting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should cache the value of userEnabled(value)
and only call it when the value changes.
} | ||
|
||
/// Initializes an action that will be conditionally enabled, and creates a | ||
/// SignalProducer for each input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SignalProducer
should be backtick-quoted.
This encapsulates the pattern where an Action should be enabled only if its input is non-nil. This moves the declaration of `Action.init` into `ActionProtocol`, so that convenience initialisers can be defined in protocol extensions.
This unifies isExecuting and userEnabled in a single struct, so that changes to either value can be made atomically. Changes no longer need to be synchronised by a serial dispatch queue, and atomically enforcing the enabledness of the action is much simpler.
The force unwap in `ActionProtocol.init(input:_:)` is a code smell warning of a possible race condition between checking the enabled state of the input property and unwrapping the input value. It's difficult to guarantee that this is safe in all conditions. This change modifies the designated initializer to allow the execute closure to send `ActionError` instead of just `Error`. This gives us the ability to safely unwrap the input property, and if the value is nil go ahead and send `ActionError.disabled`. It's not appropriate to force all consumers to change their code to return `ActionError`, so this new initialiser has to be an unambiguous overload of `Action.init(enabledIf:_:)`. Because of trailing closure syntax, the only way to make the overload unambiguous is to change the external parameter labels. This change uses the label `_internalEnabledIf:` to signal to consumers that this is an implementation detail that shouldn't be called directly. Once Swift removes the 'Same-type requirement makes generic parameter non-generic' error, we'll be able to implement `Action.init(input:_:)` as en extension on `Action` instead of `ActionProtocol`, and the internal initializer can truly be marked internal at that point.
See #88 for context. There is a race condition inherent in this code: let input: Property<Int?> // (1) input value is checked to see if it is non-nil let action = Action(enabledIf: input.map { $0 != nil }) { // (2) input value is read _again_, and could have changed concurrently let input = input.value! } Because the property is read twice, the reads are not atomic and there's no guarantee the value will be the same at both (1) and (2). This change adds the ability for `Action` to atomically observe changes to a generic `State` property. It changes `enabledIf` to be a function that returns a `Bool` based on the current state. By storing this generic, opaque state value atomically, we can read the value once, and guarantee that it has not changed between checking the `enabledIf` predicate and then passing it into the `execute` closure: let input: Action<Int?> // (1) action state is read, and the value passed to `enabledIf` let action = Action(state: input, enabledIf: { $0 != nil }) { input, _ in // (2) the same value, which was read atomically, is passed into `execute` // making this force-unwrap safe foo.bar(with: input!) } The convenience initialiser `Action(input:_:)` is now implemented in terms of this generic state initialiser, encapsulating the pattern of a property that operates on its input while non-nil.
fea3dc6
to
1672883
Compare
@andersio In order not to block this PR any further, how do you feel about submitting your proposed changes as a separate PR after this is merged? |
@mdiep I've addressed your feedback, thanks! Hopefully good to go once we're green. |
This looks great. Thanks for all the hard work on this, @sharplet! |
@sharplet Sure. |
📗 🍏 💚 |
Looks like the |
I don't think that was intentional. |
It would need to be retained even without this PR due to #117, unless we use |
Confirmed, that was unintentional. I experimented with a fix for that but shelved it until #117 was closed. |
Update: Closes #88.
PropertyAction
encapsulates the pattern where anAction
should be enabled only if its input is non-nil.The prime example of this pattern is form validation: validate some input text, and store the validated results in a property. If the form is valid, the property will be non-nil, and it should be safe to grab the current value and submit the form.
I've personally written a bunch of
Action
-based code that basically just implemented this logic in an ad-hoc fashion, so I think it would be super useful to have a convenient shorthand for it.Ideally this could just be a convenience initialiser on
Action
:But unfortunately this triggers Swift's "same-type requirement makes generic parameter non-generic" error.