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

Consider making reducer run before mutations. #10

Closed
seivan opened this issue Apr 22, 2017 · 7 comments
Closed

Consider making reducer run before mutations. #10

seivan opened this issue Apr 22, 2017 · 7 comments

Comments

@seivan
Copy link
Contributor

seivan commented Apr 22, 2017

Given

State

struct State {
  var isLoading = false
  var tasks = [Task] 
}

Actions

enum Action {
  case requestTasks
  case requestTasksCompleted([Task])
}

Dispatching .requestTasks action first runs the reducer and sets isLoading = true thus the UI can update, e.g show a spinner.
Then in the mutation (what Redux-Observable[0] calls an "epic") you can start the request and on completion dispatch .requestTasksCompleted([Task]) for the reduce tor reload the list.

As of now, you would need three actions, e.g .setLoading , where before every requestTasks manually first dispatch .setLoading.

Edit: Could even use generics to ensure that the actions for Input is not the same as Output in the mutation so you don't get a infinite loop which they can't do with JS :-)

0

@devxoul
Copy link
Member

devxoul commented Apr 22, 2017

In that case it's recommended to use both Action and Mutation. Action is just an abstraction of an user input from a view and a mutation is a state-manipulator.

enum Action {
  case requestTasks
}

enum Mutation {
  case setLoading(Bool)
  case setTasks([Task])
}

func mutate(action: Observable<Action>) -> Observable<Mutation> {
  switch action {
  case .requestTasks:
    return Observable<Mutation>.concat([
      Observable.just(.setLoading(true)), // .setLoading(true)
      fetchTasksUsingAPI().map { Mutation.setTasks($0) }, // .setTasks([...])
      Observable.just(.setLoading(false)), // .setLoading(false)
    ])
  }
}

You may refer to the actual code: https://github.com/devxoul/Drrrible/blob/f52812943d16a891ace62083adac753c2c30ac87/Drrrible/Sources/ViewControllers/ShotListViewReactor.swift#L44-L53

@seivan
Copy link
Contributor Author

seivan commented Apr 22, 2017

Yes, I understand that part. The problem is you're gong to get a lot of messy actions that don't do anything they than toggling visibility of components. Setting the loading spinner is part of loading the tasks. Not two different actions. Except one mutation is async and the other synchronous.

@seivan
Copy link
Contributor Author

seivan commented Apr 22, 2017

Btw, I figured out a way to handle the infinite loop problem That Redux-Observables have. Ensure type constraints on your "epic". T: Action -> U:Action where T != U.
That solves the :

"const actionEpic = (action$) => action$; // creates infinite loop"

@devxoul
Copy link
Member

devxoul commented Apr 22, 2017

Setting the loading spinner is part of loading the tasks. Not two different actions.

There's one action and two mutations. It's not "two different actions". One action can occur multiple mutations.

And I didn't see anywhere that occurs infinite loop in ReactorKit. Could you reproduce it?

@seivan
Copy link
Contributor Author

seivan commented Apr 22, 2017

@devxoul No, the infinite loop is part of Redux-Observable issue :) not related here, sorry for the confusion!

Yeah but why does setting loading need its own action? As far as I see, those two things are related. There are other cases where you'd want an action to reduce state and async trigger a different action.

I really suggest looking into Redux-Observable, the strength comes from the ability to cancel async actions. https://redux-observable.js.org/docs/recipes/Cancellation.html

@devxoul
Copy link
Member

devxoul commented Apr 22, 2017

Why do we have to expose an Action.requestTasksCompleted to a view? A view's interest is just an Action.requestTasks. Action and Mutation are different. Action just takes user inputs from a view and Mutation just manipulates the state. Their role are different from each other. Of course the Action.requestTasks and setLoading can be related but "setting loading" is not a role of an Action. I would like to separate the responsibility to reduce complexity.

And for cancellation, I think we can make actionSubject public. It may look like:

func mutate(action: Action) -> Observable<Mutation> {
  switch action {
  case .refresh:
    return Observable<Mutation>.concat([
      Observable.just(.setLoading(true)),
      API.requestTasks()
        .map { Mutation.setTasks($0) }
        .takeUnil(self.actionSubject.filter(Action.cancelRefreshing)),
      Observable.just(.setLoading(false)),
    ])
  }
}

I'll take a look at this for another solution 👀

@seivan
Copy link
Contributor Author

seivan commented Apr 22, 2017

Alright, I think we see this differently. From my PoV it's irrelevant what alters the state.
showing a loading spinner is exactly the same thing as setting a list of tasks.
They both update the UI that's subscribed to the state.

But I'll just maintain this notion myself! Thanks for the code example on canceling, that's useful!

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

No branches or pull requests

2 participants