pointfreeco / swift-composable-architecture

A library for building applications in a consistent and understandable way, with composition, testing, and ergonomics in mind.
https://www.pointfree.co/collections/composable-architecture
MIT License
12.07k stars 1.41k forks source link

isPresented dependency: Incorrectly set to 'true' in tests. #3121

Closed StuSharpe closed 2 months ago

StuSharpe commented 2 months ago

Description

Since 1.10.3 the isPresented dependency is 'true' when run in TestStore, in scenarios where we would expect it to be false - ie including in a parent reducer without using the navigation tools (@Presents macro).

Only seems to affect tests, works as expected in regular Store

Checklist

Expected behavior

Expect isPresented behavior for TestStore to be the same as when used in regular Store

isPresented dependency should be 'false' when used in TestStore for a child reducer that is embedded in a parent without TCA presentation logic (@Presentsmacro is not used to embed child reducer).

Actual behavior

isPresented dependency is 'true' when used in TestStore for a child reducer that is embedded in a parent without TCA presentation logic

Steps to reproduce

import Foundation
import XCTest
import ComposableArchitecture

class PresentedDismissRepo: XCTestCase {

    // This test passes for TCA 1.10.2 but fails in TCA 1.10.3 and 1.10.4
    @MainActor func testIsPresentedRepo() async {

        let store = TestStore(initialState: .init()) {
            ParentFeature()
        }

        // present the child
        await store.send(.presentChild) {
            $0.childFeature = ChildFeature.State()
        }

        // dismiss the child
        await store.send(.childFeature(.donePressed))
        await store.receive(.childFeature(.delegate(.done))) {
            $0.childFeature = nil
        }

        // present the child again
        await store.send(.presentChild) { // TCA 1.10.3 : error "Can't send action to dismissed test store"
            $0.childFeature = ChildFeature.State()
        }
    }
}

@Reducer
struct ChildFeature: Reducer {
    struct State: Equatable { }

    enum Action: Equatable {
        case donePressed
        case delegate(Delegate)
        enum Delegate {
            case done
        }
    }

    @Dependency(\.dismiss) var dismiss
    @Dependency(\.isPresented) var isPresented //<-- should be false, but is true when run in TestStore in TCA 1.10.3

    public var body: some Reducer<State, Action> {
        Reduce { state, action in
            switch action {
            case .donePressed:
                return .run { send in
                    await send(.delegate(.done))
                    if self.isPresented {
                        await self.dismiss()
                    }
                }

            case .delegate:
                return .none
            }
        }
    }
}

@Reducer
struct ParentFeature: Reducer {

    struct State: Equatable {
        var childFeature: ChildFeature.State?
    }

    enum Action: Equatable {
        case presentChild
        case childFeature(ChildFeature.Action)
    }

    public var body: some Reducer<State, Action> {

        Reduce { state, action in
            switch action {
            case .presentChild:
                state.childFeature = ChildFeature.State()
                return .none

            case .childFeature(.delegate(.done)):
                state.childFeature = nil
                return .none

            case .childFeature:
                return .none
            }
        }.ifLet(\.childFeature, action: \.childFeature) {
            ChildFeature()
        }
    }
}

The Composable Architecture version information

1.10.4 and 1.10.3

Destination operating system

test run simulator iPhone 16 Pro (17.2)

Xcode version information

Version 15.3

Swift Compiler version information

swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: arm64-apple-macosx14.0
mbrandonw commented 2 months ago

Hi @StuSharpe, thank you very much for the test. That helps a lot understand what you are seeing.

However, while I believe the library could improve upon something here, I do not actually think this is a bug in the library. The \.dismiss and \.isPresented dependencies only work when using the navigation tools that use @Presents and PresentationAction. \.dismiss can't work when not using PresentationAction because there is no distinguished "dismiss" action to be sent and clear out state.

And in fact, you are kind of seeing that in your test. You invoke dismiss() but also you are sending a delegate action to nil out state. When the dismiss tool is working properly you don't need to do the nil'ing out. It happens automatically via PresentationAction.dismiss. So I'm curious how you were led to using dismiss without using @Presents and PresentationAction?

If you were to run this code out of a test you should get purple runtime warnings letting you know that a proper presentation context was not set up, and so \.dismiss and \.isPresented can't possibly work. And ideally you would have gotten test failures for those warnings, but due to our changes in #3044 of 1.10.3 it looks like we are not swallowing those errors. I think that could be improved upon.

StuSharpe commented 2 months ago

Thank you @mbrandonw - I guess I misunderstood what\.isPresented is for.

I have a child feature that I reuse across different parents, some of which use the navigation tools (@Presents - don't need to nil out the child) and some that don't (listen for the child delegate method and nil out the state). The parent's that don't are older UIKit code that pre-dates navigation tools and has yet to be updated.

I wanted child controller logic that could conditionally dismiss() without having to know anything about its parent. But since \.isPresented relies on @Presents thats not going to work. I guess the solution in my case is to pass a bit of state from the parent to the child rather than use the \.isPresented dependency.

Whats the intended use case for \.isPresented?

mbrandonw commented 2 months ago

Whats the intended use case for .isPresented?

It's intended for what you are using it for, but only if using the full set of navigation tools. I think in the future we would get rid of the ifLet that works with non-@Presents state and non-PresentationAction actions, but that would have to wait until 2.0.

StuSharpe commented 2 months ago

OK, thanks for looking into this @mbrandonw - happy if you want to close this.

StuSharpe commented 2 months ago

I think in the future we would get rid of the ifLet that works with non-@Presents state and non-PresentationAction actions, but that would have to wait until 2.0.

Not all child features have associated UI though - would hope we could still have ifLet equivalent that doesn't use @Presents

mbrandonw commented 2 months ago

Not all child features have associated UI though - would hope we could still have ifLet equivalent that doesn't use @Presents

The "presents" terminology is just semantics. It does not have anything to do with UI or force on to have UI for a child feature. It certainly is most used with UI, but it doesn't need to be.

And even if a child feature doesn't have associated UI, it still seems handy for a child feature to be able to "dismiss" itself, i.e. tell the parent to nil out its associated state.

So we don't think we are losing much by getting rid of the non-"presents" version of ifLet. But if I am missing some potential use case then please do let us know!

mbrandonw commented 2 months ago

Hi @StuSharpe, I am going to close this for now, but please do open a discussion if you want to discuss it further. Thanks!