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

Resolve thunks #67

Closed
pburtchaell opened this issue Mar 31, 2016 · 13 comments
Closed

Resolve thunks #67

pburtchaell opened this issue Mar 31, 2016 · 13 comments

Comments

@pburtchaell
Copy link
Owner

pburtchaell commented Mar 31, 2016

With the release of 3.x and the inability to resolve thunks, the new API makes it more difficult to modify actions.

From @tomatau's comments on the original 3.0.0 PR:

how would I change the meta value between the pending and fulfilled actions? e.g. I want a special meta value only for the fulfilled but not pending or rejected.

We could either add the ability to resolve thunks or look into other options, such as DSL or a modify hook.

Problem: resolving thunks is not possible

// You can't do this
const actionCreator = () => ({
  type: 'BAR',
  payload: {
    promise: myAsyncAction().then((resolved) => 
      (action, dispatch) => {
        dispatch({ ...action, payload: { ..resolved }, meta: { broadcast: true } })
      }))
   }
});

Solution A: DSL

// example with regular meta that is dispatched in each action (pending, fulfilled/rejected)
{
  type: 'FOO',
  meta: {
    // ...
  }
}

// example with meta for each action
{
  type: 'BAR',
  meta: {
    pending: {
      // meta properties for pending action
    },
    fulfilled: {
      // meta properties for fulfilled action
    }
  }
}

Solution B: Modify Hook

A modify hook that can be supplied that sits between the resolved promise and dispatch so that the action can be modified.

const creator = () => ({
  type: 'BAR',
  payload: {
    promise: myAsyncAction(),
    modify: (action) => {
      // other logic?
      return { ...action, meta: { broadcast: true } }
    }
  },
})
@pburtchaell pburtchaell added enhancement question Issues blocked by a question labels Mar 31, 2016
@tomatau
Copy link
Collaborator

tomatau commented Mar 31, 2016

Why isn't it possible with a resolved thunk?

@pburtchaell
Copy link
Owner Author

Why isn't it possible with a resolved thunk?

Resolved thunk functionality was removed from the middleware because of the issues caused, e.g., issue #61. Perhaps I am misunderstanding though?

PS: Sorry for the delay. It has been a busy week.

@tomatau
Copy link
Collaborator

tomatau commented Apr 10, 2016

I don't follow. This issue is all about resolving objects but thunks are resolving functions, they shouldn't overlap?

@tomatau
Copy link
Collaborator

tomatau commented Apr 18, 2016

I've made a fork with a branch that keeps the thunk behaviour and also returns the original promise unmodified.

The original error always gets thrown, because it's the original promise. The middleware just creates a new promise internally for creating rejected actions.

Even errors that previously got swallowed (e.g. component errors as the result of an async dispatch) now propagate up.

https://github.com/tomatau/redux-promise-middleware/blob/another-implementation/src/index.js

I've updated the tests on this fork branch too.

@pburtchaell
Copy link
Owner Author

Since this is preserved in v2 and you have been maintaining that release, I am going to close this. Maybe you should add a note to the README though (about v2 enabling you to use thunks).

@tomatau
Copy link
Collaborator

tomatau commented May 30, 2016

I would really like to see a solution for this in a latest release -- I'm currently getting warnings from version monitoring systems that say I'm out of date with this middleware (and npm outdated isn't as pretty as it could be) :P

@pburtchaell
Copy link
Owner Author

I'm fine with adding it to v3. Do you have time to commit to implementing it in v3?

@pburtchaell pburtchaell changed the title Modifying actions? Resolve thunks in v3 May 30, 2016
@pburtchaell pburtchaell reopened this May 30, 2016
@pburtchaell pburtchaell changed the title Resolve thunks in v3 Resolve thunks May 30, 2016
@pburtchaell pburtchaell removed the question Issues blocked by a question label May 30, 2016
@pburtchaell pburtchaell added the help wanted Issues the project could use your help on label Jul 29, 2016
@rluiten
Copy link

rluiten commented Aug 24, 2016

I think I understand what you want to achieve and I did it this way.
I am using post() and get() from axios package at moment which is promise based.
I added a then(val) handler to this example, so that fulfilled results get an extra conditionFlag field set.

const OBSERVATION_COMMENT_SAVE = 'OBSERVATION_COMMENT_SAVE';

//
// In this I am capturing the comment.id and making it available on the
// rejected promise result value. This way my reducer has the info.
//
// For my case at least comment may be new and have id === 0.
// My reducer still gets that value.
//
export function saveComment(
    comment,
    newCommentText,
) {
    const { id } = comment;
    const url = id
        ? `${apiUrls.comment}/${id}`
        : `${apiUrls.comment}/`;
    const promise = post({
            url,
            data: {
                id,
                comments: newCommentText
            }
        })
        .then((val) => {
            if (myConditionFlag) {
                val.conditionFlag = true;
            }
            return val;
        });
        .catch((errorVal) => {
            if (id) {
                errorVal.id = id;
            }
            return Promise.reject(errorVal);
        });

    return {
        type: OBSERVATION_COMMENT_SAVE,
        payload: { promise, data: { id, comments: newCommentText }}
    };
}

@tomatau
Copy link
Collaborator

tomatau commented Aug 24, 2016

This doesn't solve the problem of modifying the meta information for _FULFILLED only.

Imagine you have an app that is connected through sockets and you have a middleware that listens for actions. When an action has a meta with { broadcast: true } you wish to broadcast this action and play it over all other clients connected to the socket. Now - you don't want to play all the pending or rejected actions to everyone as that's sometimes just not very useful... but you do want to broadcast the fulfilled actions!

Thunks solve this problem.

The solution already exists and is working fine in v2.

@rluiten
Copy link

rluiten commented Aug 24, 2016

If you modify .catch(errorVal) in example and use .then(val) and modify val it will modify metadata returned - I modified the example above to include a .then(val) handler to demonstrate.

I think maybe I still don't fully understand the use case you are describing from your most recent comment though, though I think now that any middleware after the action creator should be able to pick up the flag on a FULFILLED state ?

@tomatau
Copy link
Collaborator

tomatau commented Aug 25, 2016

The socket middleware reads meta data, not the action type - this is by design as we want to be explicit about the actions that are broadcast.

Your example modifies the payload, not the meta. Anything resolved by the promise is used to populate the payload.

A Flux Standard Action has the shape:

{
  type: string,
  ?payload: any,
  ?meta: object,
  ?error: bool
}

v3 and v4 of redux-promise-middleware take the meta from the original action shape and apply it both the _pending and _fulfilled or _rejected without the capability to modify per type.

@pburtchaell pburtchaell added blocked Issues blocked from progress and removed enhancement labels Dec 14, 2016
@pburtchaell pburtchaell removed blocked Issues blocked from progress help wanted Issues the project could use your help on labels Dec 21, 2016
@artemjackson
Copy link

You can normally live without thunk resolving feature, see the example: #43 (comment)

@tomatau
Copy link
Collaborator

tomatau commented Mar 8, 2017

@artemjackson not sure I follow, the example you posted is resolving with a name and a thunk!.. :)

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

4 participants