Closed Tyler-Keith-Thompson closed 3 years ago
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 OrchestrationResponder
should have the responsibility of calling the onFinish
closure at the appropriate time. We're currently doing this using a function on OrchestrationResponder
called complete
.
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:
Root -> FR1 (removeAfterProceeding) -> FR2 (removeAfterProceeding) -> COMPLETED STATE
And
Root -> FR1 (shouldLoad False) -> FR2 (shouldLoad False) -> COMPLETED STATE
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:
Workflow1 says "All my items are removedAfterProceeding", in its onFinish
I can call workflow.abandon()
if I want that nav controller gone, but I could just as easily launch Workflow2 which WANTS that nav controller there for its items.
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 UINavigationController
that we created.
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 launchInto
from that controller.
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
CallproceedInWorkflow()
on all items in that flow Observe after OnFinish is called the finalUIViewController
is still displayedExpected behavior
The
UIViewController
should not be displayed after the onFinish block has executedScreenshots
NO