Closed ArielGueta closed 7 years ago
How are your components calling either the service and/or the action creator?
In your Todo actions - every time you call todoActions.getTodos()
- you are setting up a new subscription that is never getting cleaned up.
Also, are you updating the todos in your reducer? Looking at your code - you are dispatching a GET_TODOS_SUCCESS
, but no payload - is your state getting updated appropriately to handle the success?
In my component I am calling TodosActions.getTodos()
and I just omitted the payload, but yes there is a payload.
So you are saying I need to cancel the subscription every time to get the desired effect?
The main issue here is that you appear to be thinking in an imperative programming style (much like we used to with promises and thunks).
However @angular/http
and @angular-redux/store
are both based on Observables, which requires us to think in a 'reactive' programming style.
The nearest I could come up with based on your snippet above is this:
import { Injectable } from '@angular/core';
import { Http } from '@angular/http';
import { NgRedux } from '@angular-redux/store';
import { Subscription } from 'rxjs/Subscription';
// Probably you already have this defined somewhere else.
interface ITodo {
// Whatever your todo type looks like.
}
// Probably you already have this defined somewhere else.
interface IAppState {
todos?: ITodo[];
}
class TodosService {
private todos: ITodo[];
constructor(
private ngRedux: NgRedux<any>,
private http: Http) {
// Register a callback that will fire whenever the
// value of 'todos' changes in the store. Use that
// callback to save the current value locally to the
// service.
//
// If this were a component, we'd have to remember to
// unsubscribe whenever it's destroyed; however since
// this is a service it's a singleton so I'm not going
// to worry about that.
ngRedux.select<ITodo[]>('todos').subscribe(
todos => this.todos = todos);
}
getTodos() {
// Only dispatch the relevant actions if we don't already
// have TODOs.
if (!this.todos || !this.todos.length) {
this.ngRedux.dispatch( { type: GET_TODOS } )
this.http.get(...)
.subscribe(
// The success action triggers one of your reducers to
// update store.todos I'm guessing.
res => this.ngRedux.dispatch({ type: GET_TODOS_SUCCESS, payload }),
err => this.ngRedux.dispatch({ type: GET_TODOS_FAILED, err}))
}
}
}
class TodosActions {
getTodos() {
this.TodosService.getTodos();
}
}
However this is kind of a messy way of doing it. It involves listening to the 'todos' array at the start of the TodoService lifetime (which is fine) and using a subscription to make a copy of the todos array whenever it changes in the store (which is an antipattern in reactive programming).
Then you're making a ton of one use subscriptions to Http.get which isn't really the way it's meant to be used.
Risks here include:
In my opinion, this type of logic is much better handled using an epic from redux-observable. Take a look at the epic example in our docs: https://github.com/angular-redux/store/blob/master/docs/epics.md
(see here for more info on redux-observable and the Epic pattern)
In your case, while the store-level setup will be similar, what you'd actually be doing is something more like this (untested code)
@Injectable()
export class TodosEpics {
constructor(private http: Http) {}
// An Epic listens to your action stream after all the reducers have been
// applied; it uses RxJS operators to transform that stream into another
// observable of actions.
private loadTodosEpic = (action$, store) =>
// In this case GET_TODOS has been triggered by a very simple action creator.
action$.ofType(GET_TODOS)
// Ignore GET_TODOS actions any time there are already TODOs in the state.
.filter(() => store.getState().todos && store.getState.todos.length)
// If we don't already have todos, it's time to combine this stream with
// the observable coming back from http.get()
.switchMap(a => this.http.get(...)
// It worked, fire an action that will cause the reducers to put the
// retrieved todos back in the store.
.map(data => ({ type: GET_TODOS_SUCCESS, payload: data }))
// It didn't work. Fire an action so save error info.
.catch(response => ({ type: GET_TODOS_FAILED, error: response.error })))
// This gets registered with redux.
public readonly todoLoaderMiddleware = createEpicMiddleware(this.loadTodosEpic);
}
This approach is nice for a few reasons
First, big thanks for your time and your answer. The problem with your approach is when you fire the GET_TODOS
action, your pending
key from your state is changing to true, therefore a spinner will show forever.
right - then you can make the reducer that controls the pending
field clear it on GET_TODOS_SUCCESS or on GET_TODOS_FAILURE
But when you don't pass the filter
operator either of the actions you mentioned will not call.
Oh yeah.
In that case what I've done previously is something like this:
private loadTodosEpic = (action$, store) =>
// In this case GET_TODOS has been triggered by a very simple action creator.
action$.ofType(GET_TODOS)
// Ignore GET_TODOS actions any time there are already TODOs in the state.
.filter(() => store.getState().todos && store.getState.todos.length)
// Hook up your 'pending' reducer to this action instead
.do(() => store.dispatch({ type: GET_TODOS_STARTED }))
// If we don't already have todos, it's time to combine this stream with
// the observable coming back from http.get()
.switchMap(a => this.http.get(...)
// It worked, fire an action that will cause the reducers to put the
// retrieved todos back in the store.
.map(data => ({ type: GET_TODOS_SUCCESS, payload: data }))
// It didn't work. Fire an action to save error info.
.catch(response => ({ type: GET_TODOS_FAILED, error: response.error })))
Instead of having the do
, you could do something like
private loadTodosEpic = (action$, store) =>
// In this case GET_TODOS has been triggered by a very simple action creator.
action$.ofType(GET_TODOS)
// Ignore GET_TODOS actions any time there are already TODOs in the state.
.filter(() => store.getState().todos && store.getState.todos.length)
// Hook up your 'pending' reducer to this action instead
// If we don't already have todos, it's time to combine this stream with
// the observable coming back from http.get()
.switchMap(a => this.http.get(...)
// It worked, fire an action that will cause the reducers to put the
// retrieved todos back in the store.
.map(data => ({ type: GET_TODOS_SUCCESS, payload: data }))
// It didn't work. Fire an action to save error info.
.catch(response => ({ type: GET_TODOS_FAILED, error: response.error })))
.startsWith({ type: GET_TODOS_STARTED })
Hi guys,
What would be the way to handle caching the results of my API calls? Has anybody successfully implemented caching with this library?
I tried to do something like this:
But this will be an infinite loop. Any suggestions?