Breakpoint instead of assert in optional/forEach #296
Conversation
| /// change. | ||
| /// * `Environment`: A type that holds all dependencies needed in order to produce `Effect`s, such | ||
| /// as API clients, analytics clients, random number generators, etc. | ||
| /// * `State`: A type that holds the current state of the application. |
There was a problem hiding this comment.
Just being consistent here. I added some code examples embedded in bullet points later on in this PR, which requires an indentation level of 8, so it looks best to make sure the text of bullets are indented by 4.
| /// Often used in tandem with `pullback` to transform a reducer on a non-optional local domain | ||
| /// into a reducer that can be combined with a reducer on a global domain that contains some | ||
| /// optional local domain: | ||
| /// Often used in tandem with `pullback` to transform a reducer on a non-optional child domain | ||
| /// into a reducer that can be combined with a reducer on a parent domain that contains some | ||
| /// optional child domain: |
There was a problem hiding this comment.
Any thoughts on updating our terminology from global-local to parent-child? It read better to me here. Global alone sounds like app-level domain, so you need to qualify as "more global" or "more local"...
| Warning: Reducer.optional@\(file):\(line)) | ||
| "\(debugCaseOutput(action))" was received by an optional reducer when its state was \ | ||
| "nil". This is generally considered an application logic error, and can happen for a \ |
There was a problem hiding this comment.
Qualified this with "generally" everywhere now and note that it is reasonable to ignore the result of some effects in a bullet point below.
There was a problem hiding this comment.
@stephencelis In my experience this almost exclusively happens because of erratic NavigationLink binding behavior. The action in the binding gets called multiple times, for mysterious reasons. Maybe it's worthwhile to list it as one of the sub points and an Apple bug?
There was a problem hiding this comment.
I can confirm that NavigationLinks are weird in my CA apps too, and I also blame SwiftUI 😅.
But @ferologics, doesn't something as simple as sending an action from onDisappear count?
| file: file, | ||
| line: line | ||
| ) | ||
| #if DEBUG |
There was a problem hiding this comment.
Won't check for breakpoints on release builds.
| ) | ||
| #if DEBUG | ||
| if breakpointOnNil { | ||
| fputs( |
There was a problem hiding this comment.
Using fputs to print to stderr.
| for this reducer can only be sent to a view store when state is non-"nil". In \ | ||
| SwiftUI applications, use "IfLetStore". | ||
| --- | ||
There was a problem hiding this comment.
This trailing line is needed since fputs doesn't print a trailing new line.
| /// - breakpointOnNil: Raises `SIGTRAP` signal when an action is sent to the reducer but the | ||
| /// index is out of bounds. This is generally considered a logic error, as a child reducer | ||
| /// cannot process a child action for unavailable child state. |
There was a problem hiding this comment.
I dunno if breakpointOnNil is the right name for the forEach reducers...maybe we can come up with something better.
| /// Button("Child Action") { childViewStore.send(.action) } | ||
| /// } | ||
| /// ... | ||
| /// } |
There was a problem hiding this comment.
If these additional docs look good I can create similar versions for forEach, or I can add a "see also" in forEach to read the optional documentation for more info (although I think the explanations are slightly different).
| /// process a child action for unavailable child state. | ||
| /// - Returns: A reducer that works on optional state. | ||
| public func optional( | ||
| breakpointOnNil: Bool = true, |
There was a problem hiding this comment.
Thank you for making this change - I definitely understand the reasons for the previous strict behaviour, but I think this is a nice balance. Default to strict, with the option to ignore if needed.
I've run into a particular case recently with a NavigationLink's binding that sends a "dismiss" action after the child state is nullified, and the only way I've found to work around it is to put in a delay to let the navigation complete before nulling out the child state. Ignoring the error would be preferable to the delay. Though I acknowledge that there may be something incorrect about how I'm using the navigation link that is causing the problem.
There was a problem hiding this comment.
As always, thanks for the great work 👏 .
Would you consider a setup where this takes a canIgnoreAction: (Action) -> Bool = { _ in false } parameter? Id love to retain the strict behavior for most actions and explicitly list those that can come in post mortem.
|
(No rush of course, is there a targeted dot release for this? If not, I can totally work off |
mbrandonw
force-pushed
the
dont-assert-signal
branch
from
cfc196e to
9e6c04c
Compare
|
After encountering a bug similar to this one I was only able to workaround it by switching to this branch - I'm hoping to further investigate and fix the underlying issue, but being able to fallback to this less strict behaviour is very valuable to help bypass some of the odd things that currently happen in SwiftUI. I would appreciate if this could be merged, or if you could share the reasoning for it not being merged. |
|
@nmccann We plan on merging it but we want to make sure the docs and API is right. We'll try to get things across the finish line soon. |
|
@stephencelis @mbrandonw Any progress on this? |
Based on feedback and earlier discussion.