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

redux-thunk not waiting for my promise to be fulfilled #52

Closed
alexanderharm opened this issue Feb 2, 2016 · 21 comments
Closed

redux-thunk not waiting for my promise to be fulfilled #52

alexanderharm opened this issue Feb 2, 2016 · 21 comments

Comments

@alexanderharm
Copy link

I have the following setup following your examples for chaining async actions:

// final action
function finalAction (status) {
  return {
    type: 'AUTH_STATUS'
    , status
}

// thunk action
function thunkAction () {
  return (dispatch, getState) => {
    return getStatusApi()
    .then((status) => {
      return dispatch(finalAction(status))
    })
    .catch((err) => {
      return dispatch({type: 'AUTH_ERROR'})
    })
  }
}

// api function returning another promise
function getStatusApi () {
  return myAsyncApiCall().then((res) => {
    return res.status
  })
}

// dispatch
store.dispatch(thunkAction())
.then(() => {
  // the end
})

What happens is that the flow is like this:

store.dispatch
thunkAction
getStatusApi
end
myAsyncApiCall
finalAction
reducer

For one reason or another store.dispatch resolves before myAsyncApiCall is resolved.
I either receive then a TypeError or if wrapping the whole into a promise an undefined action error.
Any idea?

@alexanderharm
Copy link
Author

I tested now with latest versions and severals actions and I always receive the following type of error if wrapped in a promise:

app/login-page.component: {"stack":"Error: Actions may not have an undefined "type" property. Have you misspelled a constant?
at dispatch (http://localhost:9000/dist/lib/redux.js:305:14)
at http://localhost:9000/dist/lib/redux-logger.js:172:29
at http://localhost:9000/dist/lib/redux-thunk.js:9:74
at dispatch (http://localhost:9000/dist/lib/redux.js:510:19)
at http://localhost:9000/dist/app/store/actions/usage.actions.js:35:20
at Object.dispatch (http://localhost:9000/dist/lib/redux-thunk.js:9:45)
at StoreService.dispatch (http://localhost:9000/dist/app/store/store.service.js:23:21)
at LoginPageComponent._navigate (http://localhost:9000/dist/app/main/login-page.component.js:145:28)
at LoginPageComponent._determineNavigationTarget (http://localhost:9000/dist/app/main/login-page.component.js:141:25)
at http://localhost:9000/dist/app/main/login-page.component.js:99:30","message":"Actions may not have an undefined "type" property. Have you misspelled a constant?","name":"Error","logData":"login"}

If I reorganise my flow into resolving the promise first and then calling a normal non-async action it works perfectly fine. I do not make any modifications getStatusApi by doing this. So I have absolutely no idea what causes these errors (Angular2, zone.js, core.js, redux-thunk, ...)?

@mmazzarolo
Copy link

I'm having the same issue since upgrading to React Native 0.19

@nicohvi
Copy link

nicohvi commented Feb 5, 2016

I'm having the same problem with async actions.

// actionCreator
export function asyncAction () {
  return dispatch => api().then(data => data.id)
}

// component
dispatch(asyncAction()).then(() => console.log('done'));
// => Cannot read property 'then' of undefined

Love the library btw 👍

@gaearon
Copy link
Collaborator

gaearon commented Feb 5, 2016

The library code has not changed since its initial release.
Please post a complete project reproducing the problem.

I have two guesses why this might be happening.

Are you using the new API without updating to Redux 3.1.0?

// Note: this API requires redux@>=3.1.0
const store = createStore(
  rootReducer,
  applyMiddleware(thunk)
);

Before 3.1.0, you had to do this:

const createStoreWithMiddleware = applyMiddleware(thunk)(createStore)
const store = createStoreWithMiddleware(rootReducer);

Are you using any store enhancers that have not updated to pass the underlying enhancer?

If you are using the new API I mentioned above, make sure you are not using any store enhancers that are not yet compatible with it. We should have addressed this better—I still plan to send PRs to popular store enhancers to allow this API.

For example, Redux DevTools must be >= 3.0.2 to use the new store creation API. Example of how that change looks like for the enhancers: reduxjs/redux#1302.

My suggestion would be to file a bug against any enhancers that you use if they don’t pass the last parameter to the underlying createStore.

In the meantime, the fix would be to use the old API if you use any enhancers that don’t support the new API yet.

@gaearon
Copy link
Collaborator

gaearon commented Feb 5, 2016

My second suggestion is incorrect. Existing store enhancers should work fine as long as you don’t mix the new and the old styles of applying them.

So this is potentially problematic (depending on whether the store enhancer has been updates to match 3.1 API):

// Potentially problematic
const finalCreateStore = someStoreEnhancer()(createStore) 
const store = finalCreateStore(reducer, applyMiddleware(thunk))

If you have code like this, either convert it fully to the old style:

// Works with any version of redux
const finalCreateStore = compose(
  someStoreEnhancer(),
  applyMiddleware(thunk)
)(createStore) 
const store = finalCreateStore(reducer)

or to the new style:

// Works only with redux >= 3.1.0
const store = createStore(
  reducer,
  compose(
    someStoreEnhancer(),
    applyMiddleware(thunk)
  )
)

Just make sure you don't mix these styles.

@gaearon
Copy link
Collaborator

gaearon commented Feb 5, 2016

In any case, there is nothing further I can do to help unless you post a complete project reproducing the issue.

@gaearon gaearon closed this as completed Feb 5, 2016
@alexanderharm
Copy link
Author

@gaearon here is a plunker demonstrating the effect:
http://plnkr.co/edit/KP3xZVH9GIdglZJLZrlb
(TypeError: Cannot read property 'then' of undefined)

The error is thrown before the promise is fulfilled. If you wrap the dispatch in a promise Promise.resolve().then... you can also follow the logging in the console.

@alexanderharm
Copy link
Author

I also tried wrapping the various parts of the async thunk into zone.bind() but it doesn't help. If you rearrange the setup, first do async then call a non-async action it works fine.

@alexanderharm
Copy link
Author

@gaearon and another plunker not involving Angular at all:
https://plnkr.co/edit/66hXPwHRi27Esp5tT79o
It doesn't throw but still then is called before the async actions resolves.

@gaearon
Copy link
Collaborator

gaearon commented Feb 5, 2016

In your second example:

      return setTimeout(() => {
        console.log('save-async-resolved')
        return dispatch(_save(name))
      }, 2000)

setTimeout does not return a Promise. It returns a timeout ID.
Therefore the returned promise is just a resolved Promise with the result equal to the timeout ID.
You don’t wait for the timeout, so it is resolved immediately.

To fix this, you want to create a Promise-friendly wrapper for timeout. For example:

function delay(ms) {
  return new Promise(resolve => setTimeout(resolve, ms))
}

// later
      return delay(2000).then(() => {
        console.log('save-async-resolved')
        return dispatch(_save(name))
      })

@gaearon
Copy link
Collaborator

gaearon commented Feb 5, 2016

The problem with the first example is exactly the same: the mistaken assumption that setTimeout returns a Promise.

Finally, note that Redux Thunk does not require you to use Promises. Thunk dispatches will return whatever you return from thunks. But Promises are usually handy so we suggest using them. Just make sure you actually return Promises and not something else.

@alexanderharm
Copy link
Author

In my real app I don't use setTimeout but a promise. I will update my plunker and come back to you.

@gaearon
Copy link
Collaborator

gaearon commented Feb 5, 2016

Please provide the exact example that fails. People spend time helping reproduce issues, so it is essential that you provide the exact code that fails rather than an approximation that fails for a different reason. Thank you!

@alexanderharm
Copy link
Author

Sorry, my fault. Am new to all of this. I updated my plunks:
non-angular (works fine!): https://plnkr.co/edit/66hXPwHRi27Esp5tT79o
angular (fails): http://plnkr.co/edit/KP3xZVH9GIdglZJLZrlb

@gaearon
Copy link
Collaborator

gaearon commented Feb 6, 2016

@alexanderharm

Thank you very much for the complete example. In your code, StoreService wraps the Redux store. However, it does not return the dispatch() return value from its own dispatch() method:

  dispatch (action): any {
    this._store.dispatch(action)
    // should have been:
    // return this._store.dispatch(action)
  }

This is why store.dispatch() returns undefined for you. Therefore, this:

    .then((res) => {
      return this._storeService.dispatch(this._nameActions.save('Redux'))
    })

does not actually wait for the async action to finish, and the promise immediately resolves.

Be careful to always return Promises if you want to wait for them 😉

@gaearon
Copy link
Collaborator

gaearon commented Feb 6, 2016

@nicohvi If you have this problem, please also post the full example reproducing it. It’s hard to say at which point return value got lost for you. It could be due to a middleware that forgets to return it, a bad middleware configuration, etc.

@alexanderharm
Copy link
Author

Damn it, I copied this from https://github.com/jhades/angular2-redux-store because I thought he would know. I will drop him a note. Thanks a million!

@gaearon
Copy link
Collaborator

gaearon commented Feb 6, 2016

Easy to overlook if they didn’t use async middleware yet. Thank you for bringing this up!

@nicohvi
Copy link

nicohvi commented Feb 10, 2016

It was a faulty store setup @gaearon, thank you very much for pointing me in the right direction 👍

@petermikitsh
Copy link

petermikitsh commented Jun 23, 2016

To add to @gaearon's example, if you need to pass in initialState, per the createStore api documentation:

// Works only with redux >= 3.1.0

let initialState = {};

const store = createStore(
  reducer,
  initialState,
  compose(
    someStoreEnhancer(),
    applyMiddleware(thunk)
  )
)

This got me a little tripped up, so I hope it helps someone else.

@kellyrmilligan
Copy link

is compose needed anymore? the docs don't mention it.

thanks.

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

6 participants