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
removedAfterProceeding on last item in Workflow #19
Comments
this bug appears to be addressed in #25 |
@Tyler-Keith-Thompson We're expecting this was fixed in Release 3.0.5 as part of the fix for Issue #25. |
It isn't, here is a test to repro: func testWorkflowCanDestroyAllItems_AndStillProceedThroughFlow_AndCallOnFinish() {
class FR1: TestViewController { }
class FR2: TestViewController { }
class FR3: TestViewController { }
let root = UIViewController()
root.loadForTesting()
let expectOnFinish = self.expectation(description: "onFinish called")
root.launchInto(Workflow(FR1.self, flowPersistence: .removedAfterProceeding)
.thenProceed(with: FR2.self, flowPersistence: .removedAfterProceeding)
.thenProceed(with: FR3.self, flowPersistence: .removedAfterProceeding)) { _ in
XCTAssertUIViewControllerDisplayed(isInstance: root) // FAILS
XCTAssertNil(UIApplication.topViewController()?.presentingViewController) // FAILS
expectOnFinish.fulfill()
}
XCTAssertUIViewControllerDisplayed(ofType: FR1.self)
XCTAssert(UIApplication.topViewController()?.presentingViewController === root)
(UIApplication.topViewController() as? FR1)?.proceedInWorkflow(nil)
XCTAssertUIViewControllerDisplayed(ofType: FR2.self)
XCTAssert(UIApplication.topViewController()?.presentingViewController === root)
(UIApplication.topViewController() as? FR2)?.proceedInWorkflow(nil)
XCTAssertUIViewControllerDisplayed(ofType: FR3.self)
(UIApplication.topViewController() as? FR3)?.proceedInWorkflow(nil)
wait(for: [expectOnFinish], timeout: 3)
} |
This may be a tad complicated, because it'll involve looking at OrchestrationResponder and potentially adding a new action |
Also when you're fixing this consider adding a test for a nav stack workflow as well. |
We have a spike branch for this called WIP_removedAfterProceeding. Currently heading in the direction that We believe the desired API behavior is: Launching a workflow with a presentationType of navigationStack should behave the same if every screen in workflow is shouldLoad == false and Completing a workflow with all screens as persistence of RemovedAfterProceeding |
Not sure I entirely understand the navigationStack thing but I'll caution you that if your thought process is "is doesn't make sense to ask for a navigation stack where every view should not load" that should be something you should make users of the library be explicit about. If you decide "in the case of a navigationStack where all views shouldLoad return false I'll behave as though you said removedAfterProceeding" you may be violating one of our tenants of good API practices and baking in some magic. Because the user of the API didn't ask for "removedAfterProceeding". We should avoid that magic wherever possible. That being said I may have drastically misunderstood your comment here. |
You did. We're working through some of the situations that arise from cleaning out the last VC in a navigation stack because of the ".removedAfterProceeding" being applied to all views. So the end user experience should be similar to that of a workflow that did not have any views that should load. Example:
These completed states should be the same. You should be viewing Root. Hopefully that makes more sense. |
This helped clarify. You aren't behaviorally treating them the same, you're noting that the end state is the same. I totally get that. |
Weird edge case to consider: What if I wanted an empty nav controller there? Example: If you dismiss the nav controller on behalf of the user you may get a weird animation experience where it dismisses, then they launch the next workflow and a new one presents. Which option provides greater flexibility? |
I think it boils down to: We should clean up the If a user wants to have a shared navigation controller between chained workflows, the more explicit code would be to create a navigation controller and then |
That makes a lot of sense. The user has the choice to create an empty NavController, then launch from that. The flexibility is UIKit flexibility and completely in their control. |
Describe the bug
If you have
.removedAfterProceeding
as theFlowPersistence
on an item in a workflow it does not properly remove that item (UIKit)To Reproduce
Steps to reproduce the behavior:
Create 3
FlowRepresentable
s the last of which has aflowPersistence
of.removedAfterProceeding
Call
proceedInWorkflow()
on all items in that flowObserve after OnFinish is called the final
UIViewController
is still displayedExpected behavior
The
UIViewController
should not be displayed after the onFinish block has executedScreenshots
NO
The text was updated successfully, but these errors were encountered: