redux-observable / redux-observable

RxJS middleware for action side effects in Redux using "Epics"
https://redux-observable.js.org
MIT License
7.85k stars 466 forks source link

TypeError: Cannot read property 'Symbol(Symbol.iterator)' of undefined #68

Closed casoetan closed 8 years ago

casoetan commented 8 years ago

Hi @jayphelps thanks for the great work here and EPICS sounds just about right.

Here's another issue. I guess this is me as well, but ...

I've updated my epics (I switched immediately) based on our last conversation #64

import Rx from 'rxjs'
import { todoHz } from '../db'

import {
  getTodosSuccessful
} from '../actions/todos'

const watchTodosEpic = action$ =>
  action$
    .ofType('request todos')
    .merge(todoHz.watch())
    .do(todos => delete todos.type)
    .map(todos => getTodosSuccessful(todos))
    .catch(err => console.log(err)) // eslint-disable-line

const addTodoEpic = action$ =>
  action$
    .ofType('request post todo')
    .takeUntil(action$.ofType('cancel requests'))
    .mergeMap(action => { todoHz.store(action.payload) })
    .catch(err => console.log(err)) // eslint-disable-line

const deleteTodoEpic = action$ =>
  action$
    .ofType('request delete todo')
    .takeUntil(action$.ofType('cancel requests'))
    .mergeMap(action => { todoHz.remove(action.payload) })
    .catch(err => console.log(err)) // eslint-disable-line

export default [watchTodosEpic, addTodoEpic, deleteTodoEpic]

The error

TypeError: Cannot read property 'Symbol(Symbol.iterator)' of undefined

seems to come from the mergeMap. Returning from that line breaks future actions, not returning works, but that error. Another check hopefully.

Cheers

jayphelps commented 8 years ago

Same problem as before. You're using arrow functions with curlies but not returning anything. Your mergeMap's must return an observable to merge.

casoetan commented 8 years ago

Hi @jayphelps Yes. When I return from the mergeMaps, subsequent actions are not caught.

Meaning I can only add one todo. Any future calls to add todo or delete todos just doesn't get to the epics. This is the reason I use the curly braces which work but the said error

jayphelps commented 8 years ago

@casoetan without runnable code I can't be sure what your exact issue is, however I do know that you are also not returning an observable from .catch() which requires you return a new stream to use instead. https://github.com/ReactiveX/rxjs/blob/master/src/operator/catch.ts#L5-L14

I imagine it's a simple misunderstanding of what catch() is for. It's not like .do() for transient side effects on errors. There isn't an operator like that AFAIK.

I don't know if this is causing your current issue though. I wouldn't think it would, unless you're hitting that error path.

Can you try reducing your code until all your actions are matched? You can use ignoreElements() to make Rx throw away any emitted values, so they don't get dispatched (while you're debugging)

e.g.

const addTodoEpic = action$ =>
  action$
    .ofType('request post todo')
    .do(action => console.log('action', action))
    .ignoreElements();

Then try

const addTodoEpic = action$ =>
  action$
    .ofType('request post todo')
    .takeUntil(action$.ofType('cancel requests'))
    .do(action => console.log('action', action))
    .ignoreElements();

Then

const addTodoEpic = action$ =>
  action$
    .ofType('request post todo')
    .takeUntil(action$.ofType('cancel requests'))
    .mergeMap(action => todoHz.remove(action.payload))
    .do(action => console.log('action', action))
    .ignoreElements();

That will help you narrow down where you're code is going wrong. Since everything else (besides the catch) looks correct, the issue may be with what you're doing in todoHz.remove(action.payload) (or what you're returning from it)

casoetan commented 8 years ago

@jayphelps testing this now.

Also todoHz returns an observable as well

Thanks

casoetan commented 8 years ago

This is queer.

As long as i have ignoreElements() at the end of the epics, it works fine. Maybe I need to create a plunker to demonstrate this.

This is what worked

import Rx from 'rxjs'
import { todoHz } from '../db'

import {
  getTodosSuccessful,
  todosFailed
} from '../actions/todos'

const watchTodosEpic = action$ =>
  action$
    .ofType('request todos')
    .merge(todoHz.watch())
    // .do(todos => delete todos.type)
    .map(todos => getTodosSuccessful(todos))
    .catch(err => console.log(err)) // eslint-disable-line

const addTodoEpic = action$ =>
  action$
    .ofType('request post todo')
    .takeUntil(action$.ofType('cancel requests'))
    .mergeMap(action => todoHz.store(action.payload))
    .catch(err => todosFailed(err))
    .ignoreElements()

const deleteTodoEpic = action$ =>
  action$
    .ofType('request delete todo')
    .takeUntil(action$.ofType('cancel requests'))
    .mergeMap(action => todoHz.remove(action.payload))
    .catch(err => todosFailed(err))
    .ignoreElements()

export default [watchTodosEpic, addTodoEpic, deleteTodoEpic]

PS. todoHZ is an http://horizon.io/ model watching the todo model horizon('todo') so it's an observable as well, reason for mergeMap.

Thanks for all the help.

Can't wait for the full update to epics.

PS. Should I be using ignoreElements this way?

Cheers

jayphelps commented 8 years ago

@casoetan you still having the issue? We recently added decent docs, so feel free to peek at them for the core concepts https://redux-observable.js.org/docs/basics/Epics.html

You definitely should not be using ignoreElements() for [nearly] any real world Epic. There are some edge cases, but for the most part it's just helpful for debugging. It prevents your Epic from emitting anything, so no actions from that Epic will ever reach your reducers.

Here are some possible insights based on your code. We now elaborate on these in the new docs as well.

I'm seeing that you have a catch() and takeUntil() in there, but both of them should really be inside your mergeMap(), after the side effect calls (which I assume are AJAX requests). That's because in Rx, operators apply to the thing you call them on. So if you takeUntil() the outside stream, that Epic will stop listening for actions completely if it receives that cancel action. Likewise, if you have an AJAX error inside the mergeMap() but you let it bubble out to the catch() on the outer stream, the original action$.ofType() will terminate and instead defer to the stream you provide in catch()....however, you're actually not providing one in the case of watchTodosEpic, which is also a problem. If you just want to log on error, use .do({ error: err => console.log(error)

Please let us know if this helps and if your issues continue.

casoetan commented 8 years ago

Thanks for the update @jayphelps

I've updated my codes to reflect what you stated. Hope I'm on track What I noticed.

import Rx from 'rxjs'
import { todoHz } from '../db'

import {
  getTodosSuccessful
} from '../actions/todos'

import {
  showNotification
} from '../actions/app'

const watchTodosEpic = action$ =>
  action$
    .ofType('request todos')
    .merge(
      todoHz
        .watch()
        .catch(err => showNotification(err))
    )
    .map(todos => getTodosSuccessful(todos))

const addTodoEpic = action$ =>
  action$
    .ofType('request post todo')
    .mergeMap(action =>
      todoHz.store(action.payload)
        .takeUntil(action$.ofType('cancel requests'))
        .catch(err => showNotification(err))
    )
    .ignoreElements()

const deleteTodoEpic = action$ =>
  action$
    .ofType('request delete todo')
    .mergeMap(action =>
      todoHz.remove(action.payload))
      .takeUntil(action$.ofType('cancel requests')
      .catch(err => showNotification(err))
    )
    .ignoreElements()

export default [watchTodosEpic, addTodoEpic, deleteTodoEpic]
jayphelps commented 8 years ago

@casoetan What does showNotification(err) return? .catch() expects you to return an observable, but I see that you import it from '../actions/app' which makes me suspect it returns a regular action object. If it does, you must wrap it into an observable, which can be done like this .catch(err => Observable.of(showNotification(err)))

What does todoHz.remove(action.payload)) return? With the code the way it stands, it must return an observable, the output of which is what currently will be dispatched as-is since you do not have any .map() after it.

Are you receiving any errors? You mention some of them fail, but in what way? Also, how familiar are you with RxJS? It's really important to have a solid basis of RxJS before using redux-observable so if the Rx stuff is tripping you up, highly recommend you check out some of the various online materials on Rx. There are a ton, some are listed here http://reactivex.io/tutorials.html#rxjs

Other than that, I'm afraid I can't be much more help without knowing what all your helper methods do and return. You best bet is to start with a single Epic that isn't working as you expect and start it over from scratch. These seem to be fairly standard Epics, so my best thought is that your helpers like todoHz.remove(action.payload) todoHz.store(action.payload) are not observables or are not emitting the correct actions as-is.

Is it possible to extract this stuff into a JSBin for me? We have several simple bin examples you can use as a starting point: http://redux-observable.js.org/docs/recipes/UsageWithUIFrameworks.html

I also put together for you a fairly basic demo of something similar. http://jsbin.com/lirazu/edit?js,output Since I don't have a real API for this it fakes it, so do keep that in mind when reading the code but otherwise this is how you'd do it.


Oh I just noticed that your last Epic also may have an extraneous closing parenthesis and missing one later too.

const deleteTodoEpic = action$ =>
  action$
    .ofType('request delete todo')
    .mergeMap(action =>
      todoHz.remove(action.payload)) // <----- this closes the mergeMap!
      .takeUntil(action$.ofType('cancel requests') // <----- missing closing one too!
      .catch(err => showNotification(err))
    )

It's possible you didn't copy-paste and just have the typo here in your comment, but wanted to point that out.

casoetan commented 8 years ago

I'll look at creating a jsbin as you mentioned. You are right, the show notification returns an action to be dispatched on error. The todoHz.remove and todoHz.store return observables. I'm using horizon.io for backend access. I'll review my codes based on your thoughts when I get to a system.

casoetan commented 8 years ago

Hi @jayphelps. Thanks for the help, I have gotten a better understanding of how epics work, and honestly, it's quite powerful.

todoHz.watch()
todoHz.store()
todoHz.remove()

all return observables.

You definitely should not be using ignoreElements() for [nearly] any real world Epic. There are some edge cases, but for the most part it's just helpful for debugging. It prevents your Epic from emitting anything, so no actions from that Epic will ever reach your reducers

  • The watchTodosEpic is called once the application starts and adds a watch on the todoHz service which receives updates from the db. Every time a new todo is added, either through the app, or from a backend, todoHz knows, then the reducer is called and the app is updated.

This works for me. Hope I'm not going to have issues going forward. I would like to have your thoughts before closing this and thanks again for all the help and assistance

My working Epic.

import Rx, { Observable } from 'rxjs'
import { todoHz } from '../db'

import {
  getTodosSuccessful
} from '../actions/todos'

import {
  showNotification
} from '../actions/app'

const watchTodosEpic = action$ =>
  action$
    .ofType('request todos')
    .merge(
      todoHz
        .watch()
        .catch(err => Observable.of(showNotification(err)))
    )
    .do(todos => delete todos.type)
    .map(todos => getTodosSuccessful(todos))

const addTodoEpic = action$ =>
  action$
    .ofType('request post todo')
    .mergeMap(action =>
      todoHz
        .store(action.payload)
        .takeUntil(action$.ofType('cancel requests'))
        .catch(err => Observable.of(showNotification(err)))
    )
    .ignoreElements()

const deleteTodoEpic = action$ =>
  action$
    .ofType('request delete todo')
    .mergeMap(action =>
      todoHz
        .remove(action.payload)
        .takeUntil(action$.ofType('cancel requests'))
        .catch(err => Observable.of(showNotification(err)))
    )
    .ignoreElements()

export default [watchTodosEpic, addTodoEpic, deleteTodoEpic]
jayphelps commented 8 years ago

@casoetan very glad things are working now.

In regards to ignoreElement(), the biggest issue is that this will in fact prevent your showNotification() actions from reaching the reducers too. Where it is currently placed, it will prevent everything from being emitted for that Epic.

You can solve that by moving where you want elements ignored to immediately after the todoHz.remove() but before the .catch()

const deleteTodoEpic = action$ =>
  action$
    .ofType('request delete todo')
    .mergeMap(action =>
      todoHz
        .remove(action.payload)
        .ignoreElements()
        .takeUntil(action$.ofType('cancel requests'))
        .catch(err => Observable.of(showNotification(err)))
    );

Now this is saying: match delete actions and map them into a todoHz.remove() request. Ignore the response if it is successful, but if it errors emit the action returned by showNotification(err).

The reason I said you usually won't use ignoreElement() (even in this adapted way above) is because devs usually want to know that some side effect, like deleting the todo, was successful. As the code is, your app will only know if it failed explicitly, which if that doesn't happen it doesn't necessarily mean it succeeded. Also, people often in the UI display some sort of pending indicator or prevent them from doing something while the side effect is still pending. Only you can know if this isn't necessary for your particular app, so if it isn't, this usage of .ignoreElements() is perfectly acceptable.

casoetan commented 8 years ago

Thanks @jayphelps. I'll be updating my usage soon.

PS. As I could not create a jsbin (Horizon and DB) I created a repo here casoetan/react-horizon. You may want to take a look at it here.

Thanks, I'll be closing this issue now.

jayphelps commented 8 years ago

@casoetan you're welcome! Very cool, will check it out.