Skip to content
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 store's action queue state on ViewStore and modify private only setter #64

Merged
merged 3 commits into from
Jan 7, 2024
Merged

Conversation

sobabear
Copy link
Contributor

@sobabear sobabear commented Jan 6, 2024

Related Issues 💭

I have two suggestion's for ViewStore.

One is making a limitation for allowing setter on public.
The other one is let viewstore be able to know of store's state, especially actionQueue.

Description 📝

Additional Notes 📚

Checklist ✅

  • If the changes affect existing functionality, please verify whether the above information has been appropriately described.

Copy link
Owner

@DevYeom DevYeom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you first suggested was necessary. Thanks!
However, I don't understand the need for your second suggestion about actionQueue and isProcessing. Could you explain why it is needed?

Note

Directly modifying the state of the store can lead to unintended side effects and is considered an anti-pattern. It is preferable to change the state only by calling send().

Sources/OneWay/ViewStore.swift Show resolved Hide resolved
@sobabear
Copy link
Contributor Author

sobabear commented Jan 7, 2024

@DevYeom
Thanks to reply!

The reason of second one, just my opinion, is that i thought Combination with 'queue-based' system and Unidirectional system could be let know developers proper informations of state.

I already know Store does automatically manage actions(queue based) so user(developer) does not need to handle it at all. Moreover some syntax shall help it such as states: AsyncStream. On the others hands when handle very long timed action (which let store is on processing and actionqueue is not empty), it would be very helpful to developer to know least information of store's state

@DevYeom
Copy link
Owner

DevYeom commented Jan 7, 2024

Your second suggestion also makes sense. However, There is a way to find out if a long-duration action is being processed could be as follows.

func reduce(state: inout State, action: Action) -> AnyEffect<Action> {
// ...
    case .request:
        return .concat(
            .just(.setIsLoading(true)),
            .single {
                let result = await longTimeAPI.request()
                return .response(result)
            },
            .just(.setIsLoading(false))
        )
    case .setIsLoading(let isLoading):
        state.isLoading = isLoading
        return .none
    }
// ...
}

I also have a plan to support a debug print feature(e.g. printing changes in state to the console). I hope you understand that I try not to expose the interface externally(public) as much as possible to reduce misuse.

How about only including the first suggestion this time?

@sobabear
Copy link
Contributor Author

sobabear commented Jan 7, 2024

Your second suggestion also makes sense. However, There is a way to find out if a long-duration action is being processed could be as follows.

func reduce(state: inout State, action: Action) -> AnyEffect<Action> {
// ...
    case .request:
        return .concat(
            .just(.setIsLoading(true)),
            .single {
                let result = await longTimeAPI.request()
                return .response(reulst)
            },
            .just(.setIsLoading(false))
        )
    case .setIsLoading(let isLoading):
        state.isLoading = isLoading
        return .none
    }
// ...
}

I also have a plan to support a debug print feature(e.g. printing changes in state to the console). I hope you understand that I try not to expose the interface externally(public) as much as possible to reduce misuse.

How about only including the first suggestion this time?

Very acceptable decision! Please check it out! @DevYeom

Copy link
Owner

@DevYeom DevYeom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@DevYeom DevYeom merged commit 35cbeb4 into DevYeom:main Jan 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants