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

Waiting for a saga to complete #697

Closed
eirslett opened this issue Dec 14, 2016 · 17 comments
Closed

Waiting for a saga to complete #697

eirslett opened this issue Dec 14, 2016 · 17 comments
Labels

Comments

@eirslett
Copy link

The use case is a legacy application without Redux, where you try to introduce (gradually) Redux and Redux-Saga without rewriting the whole app at once.
The legacy code uses a promise-based pattern:

getArticle(123).then(data => render(data)).catch(err => renderError(err))

Inside getArticle there is a fetch call to some JSON endpoint. This could be rewritten in redux:

function getArticle(id) {
  store.dispatch(actions.requestArticle(id));
}

// ...

function* requestArticle ({id}) {
  try {
    const response = yield call(fetch, 'http://example.com/...');
    const data = yield call(response.json);
    yield put('ARTICLE_REQUEST_SUCCESS', data);
  } catch (err) {
    yield put('ARTICLE_REQUEST_FAILED');
  }
}

function* saga () {
  yield fork(takeEvery, 'REQUEST_ARTICLE', requestArticle);
}

What we want to achieve, is to keep the old behavior of getArticle, so that it returns a Promise that is resolved with the data, or rejected with the error. That way, we can move parts of the app to redux while keeping other, promise-based code around for a bit longer.
I suppose this would work similar to dispatching redux thunks. Is there an easy way to build a "bridge" like that, for redux-saga?

@Andarist
Copy link
Member

Andarist commented Dec 15, 2016

Im not exactly sure what you mean by the bridge here, but this API https://redux-saga.github.io/redux-saga/docs/api/index.html#runsagaiterator-options helps connecting the saga to the non-redux projects.

In that way you could pass success/error from the getArticle's callbacks to the saga.

Or you could just dispatch your getArticle and play around with this snippet

function getArticle(id) {
  const apiCall = fetch(...); // your promise
  // you cant test your created promise origin, so checking in saga just for smth like takeEvery(ac => typeof ac.then === 'function') wont suffice as you probably like to handle different promises in different ways, so you need to have a 'type' here os some other field, but with type you can use a shortcut syntax for takeEvery('REQUEST_ARTICLE' )
  apiCall.type = 'REQUEST_ARTICLE' 
  return apiCall
}

// ...

function* requestArticle (promise) {
  try {
    const response = yield promise; // you can yield promises just like that
    const data = yield call(response.json);
    yield put('ARTICLE_REQUEST_SUCCESS', data);
  } catch (err) {
    yield put('ARTICLE_REQUEST_FAILED');
  }
}

function* saga () {
  yield fork(takeEvery, 'REQUEST_ARTICLE', requestArticle);
}

@eirslett
Copy link
Author

eirslett commented Dec 28, 2016

I ended up using this middleware: https://github.com/Chion82/redux-wait-for-action. It looks like it works quite well combined with redux-saga.

const actionCreators = {
  requestArticle: id => ({
    type: 'REQUEST_ARTICLE',
    id,
    [WAIT_FOR_ACTION]: 'ARTICLE_REQUEST_SUCCESS',
    [ERROR_ACTION]: 'ARTICLE_REQUEST_FAILED'
  })
};

store.dispatch(actionCreators.requestArticle(123))
  .then(() => console.log('success'))
  .catch(() => console.error('error'));

@Andarist
Copy link
Member

How do you leverage redux-saga in combination with this other middleware?

@eirslett
Copy link
Author

eirslett commented Dec 28, 2016

Here is an example of redux-saga combined with redux-wait-for-action, interacting with legacy code. It's pseudocode, I haven't actually tried it.

// saga code
function* requestArticleSaga({ id }) {
  try {
    const response = yield call(fetch, `https://api.example.com/${id}`);
    const data = yield call(response.json);
    yield put({ type: 'ARTICLE_REQUEST_SUCCESS', id, data });
  } catch (error) {
    yield put({ type: 'ARTICLE_REQUEST_FAILED', id, error });
  }
}

function* root() {
  yield fork(takeEvery, 'REQUEST_ARTICLE', requestArticleSaga);
}
sagaMiddleware.run(root);

// reducer
switch (action.type) {
  case 'REQUEST_ARTICLE':
    return { ...state, [action.id]: { loading: true } };
  case 'ARTICLE_REQUEST_SUCCESS':
    return { ...state, [action.id]: action.data };
  case 'ARTICLE_REQUEST_FAILED':
    return { ...state, [action.id]: { error: action.error } };
}

// action creators
function requestArticle(id) {
  return {
    type: 'REQUEST_ARTICLE',
    id,
    [WAIT_FOR_ACTION]: 'ARTICLE_REQUEST_SUCCESS',
    [ERROR_ACTION]: 'ARTICLE_REQUEST_FAILED'
  };
}

// legacy code
const id = parseInt(params.id);
view.renderLoadingSpinner();
store.dispatch(requestArticle(id))
  .then(() => view.renderArticleView(store.getState()[id]))
  .catch(view.showErrorPage(store.getState()[id].error));

I have one concern though; if you dispatch multiple REQUEST_ARTICLE in parallel, and they return responses out of order, the wrong promise might be resolved.

@Andarist
Copy link
Member

Andarist commented Dec 28, 2016

I might not know something about requirements, but I dont see why this logic couldnt be embedded in the saga, instead of in redux-wait-for-action, which would automatically solve your concern about promise order.

// saga code
function* requestArticleSaga(view, { id }) {
  yield call([view, view.renderLoadingSpinner]);
  
  try {
    const response = yield call(fetch, `https://api.example.com/${id}`);
    
    // aint sure if this method needs the appropriate this binding but it would be safer to use
    // 1. const data = yield call([response, response.json]);
    // or 2. const data = yield apply(response, response.json);
    
    const data = yield call([response, response.json]);
    yield put({ type: 'ARTICLE_REQUEST_SUCCESS', id, data });
    const articleState = yield select(state => state[id]);
    yield call([view, view.renderArticleView], articleState);
  } catch (error) {
    yield put({ type: 'ARTICLE_REQUEST_FAILED', id, error });
    const articleError = yield select(state => state[id].error);
    yield call([view, view.showErrorPage], articleError);
  }
}

function* root(view) {
  // with v0.14 if you import takeEvery from the redux-saga/effects
  // you can shorten this

  yield takeEvery('REQUEST_ARTICLE', requestArticleSaga, view)

  // also if you put the logic into the saga it could be better 
  // (if this meets your requirements) to use takeLatest instead of takeEvery  
}
sagaMiddleware.run(root, view);

// reducer
switch (action.type) {
  case 'REQUEST_ARTICLE':
    return { ...state, [action.id]: { loading: true } };
  case 'ARTICLE_REQUEST_SUCCESS':
    return { ...state, [action.id]: action.data };
  case 'ARTICLE_REQUEST_FAILED':
    return { ...state, [action.id]: { error: action.error } };
}

// action creators
function requestArticle(id) {
  return {
    type: 'REQUEST_ARTICLE',
    id,
  };
}

// legacy code
const id = parseInt(params.id);
store.dispatch(requestArticle(id))

This way the whole logic is contained in one async flow inside the saga and you get rid of the overhead tracking this between files and concepts.

When you migrate your views to be redux-connected you will just get rid of those pesky view calls inside the saga, as your views will rerender themselves automatically based on the store's state.

@eirslett
Copy link
Author

The requirement is: this is a gradual legacy code base migration; no "stop the line and rewrite the whole app to redux and redux-saga", all at once.

Calling view.xxx cannot happen inside the saga, because the requestArticle function is exported as a form of public API (you could think of it as an ArticleService). And the state management code shouldn't know about the UI code, architecture wise, the UI should know about the state management (directly or indirectly). The thought is that redux-saga will know about redux, the "facade api" will know about redux (and redux-saga), while the rest of the app (the UI) will only know about the facade.

Then, gradually, views can be shifted from using the promise APIs to reading directly from Redux instead.

@Andarist
Copy link
Member

Andarist commented Dec 28, 2016

In that case the approach with redux-wait-for-action seems really constrained and your objections towards the promise resolution order is just too big of a concern to ignore it.

I would suggest to make your own solution with a custom middleware like this:

export const DEFERRED = Symbol('DEFERRED');

const createExposedPromise = () => {
  const deferred = {}

  const promise = new Promise((resolve, reject) => {
    deferred.resolve = resolve
    deferred.reject = reject
  })

  return [ promise, deferred ]
}

export default store => next => action => {
  if (!action[DEFERRED]) {
    return next(action);
  }

  const [ promise, deferred ] = createExposedPromise()
  next({ ...action, [DEFERRED]: deferred })
  return promise
};

and then use it in the similar way like you have proposed

// saga code
function* requestArticleSaga({ id, [DEFERRED]: deferred }) {
  try {
    const response = yield call(fetch, `https://api.example.com/${id}`);
        
    const data = yield call([response, response.json]);
    yield put({ type: 'ARTICLE_REQUEST_SUCCESS', id, data });
    const articleState = yield select(state => state[id]);
    deferred.resolve(articleState)
  } catch (error) {
    yield put({ type: 'ARTICLE_REQUEST_FAILED', id, error });
    const articleError = yield select(state => state[id].error);
    deferred.reject(articleError)
  }
}

function* root() {
  yield takeEvery('REQUEST_ARTICLE', requestArticleSaga)
}
sagaMiddleware.run(root);

// reducer
switch (action.type) {
  case 'REQUEST_ARTICLE':
    return { ...state, [action.id]: { loading: true } };
  case 'ARTICLE_REQUEST_SUCCESS':
    return { ...state, [action.id]: action.data };
  case 'ARTICLE_REQUEST_FAILED':
    return { ...state, [action.id]: { error: action.error } };
}

// action creators
function requestArticle(id) {
  return {
    type: 'REQUEST_ARTICLE',
    id,
    [DEFERRED]: true,
  };
}

// legacy code
view.renderLoadingSpinner()
const id = parseInt(params.id);
store.dispatch(requestArticle(id))
  .then(articleState => view.renderArticleView(articleState))
  .catch(articleError => view.showErrorPage(articleError))

Much simpler and it wont suffer from the ordering issue

@eirslett
Copy link
Author

eirslett commented Dec 28, 2016

Yes, that would work!
The only thing that is a little ugly (in some sense) is the userland handling of Promise resolve/reject inside the saga. Would it be possible to do it as an effect somehow?

// saga code
function* requestArticleSaga({ id }) {
  try {
    const response = yield call(fetch, `https://api.example.com/${id}`);
        
    const data = yield call([response, response.json]);
    yield put({ type: 'ARTICLE_REQUEST_SUCCESS', id, data });
    yield resolve(data);
  } catch (error) {
    yield put({ type: 'ARTICLE_REQUEST_FAILED', id, error });
    yield reject(error);
  }
}

The effects resolve and reject would then resolve/reject the promise to be returned from store.dispatch(getArticle(123)). (And could be unit tested easily, one wouldn't need to mock a Promise)

Or maybe this all boils down to needing to pass an explicit promise reference, so that the correct promise is resolved...

@Andarist
Copy link
Member

Or maybe this all boils down to needing to pass an explicit promise reference, so that the correct promise is resolved...

exactly that, there needs to be some kind of connection between the requestArticleSaga instance (spawned by takeEvery) and the trigger-action's promise. With the ability to make custom effects it maybe could be somehow tied behind the scenes, but there is no possibility to make them right now.

Thats why I've proposed attaching the deferred as metadata on the action object, so it can be easily referenced. If you would like to make it more effect-like you can wrap those resolve/reject calls with call effect, but thats best what can be done for now I guess.

In general when I think about it right now such custom effects could be quite easily created if only certain internal building blocks would be exposed for people wanting to build more personalized runners. For now it will stay as open question if the internals could fulfill such premise, aint sure if they are modularized enough for now. The excellent example of hardcore modularization - exposing both high-level and low-level APIs were recently done in react-redux (v5 !). I think this migration from v4 to v5 in that package might be an excellent study case for such endavours.

@Andarist
Copy link
Member

Going to close this, if you have any further questions - please just ask :)

@webberwang
Copy link

Couldn't you just use

yield take ('MY_ACTION')

It will block on take until that action is dispatched

@artemjackson
Copy link

artemjackson commented Feb 16, 2018

Follow-up to #697 (comment)

Take a look at https://github.com/diegohaz/redux-saga-thunk

@jessepinho
Copy link

jessepinho commented Apr 11, 2018

A solution I'm using: create a super-simple middleware that listens for Redux actions that indicate "done-ness," and then calls a provided callback that renders/sends the page to the client:

// store.ts
export const getNewStore = (done: () => void): Store<State> => {
  const doneMiddleware: Middleware = store => next => action => {
    const result = next(action)

    if (action.type === FOO_LOADED || FOO_NOT_FOUND) {
      done()
    }

    return result
  }

  return createStore<State>(
    rootReducer,
    initialState,
    applyMiddleware(doneMiddleware),
  )
}
// server.ts
export default function(req: Request, res: Response, next: NextFunction) {
  let store: Store<State>

  const done = () => {
    const state = store.getState()

    if (state.Foo.isLoading === false) {
      if (state.Foo.notFound) {
        res.status(404)
      }

      res.send(someHTML)
    } else {
      res.status(500)
    }
  }

  store = getNewStore(done)

  // Dispatch the async action. The middleware will call `done()` once the async action has succeeded or failed.
  store.dispatch(loadFoo())
}

@lvdang
Copy link

lvdang commented May 8, 2018

@Andarist
Can you help me clarify this statement? Since we are in a Generator function, yield will await for promise to resolve when declared like this: yield promise ?

const response = yield promise; // you can yield promises just like that

@Andarist
Copy link
Member

Andarist commented May 8, 2018

Yeah, saga will wait for such yielded promise. This is not recommended though as promises are eager - use yield call(() => promise) instead.

@lvdang
Copy link

lvdang commented May 14, 2018

@Andarist thanks for the clarification. Didn't know that, I often use call(), is "yield promise" documented anywhere?

@Andarist
Copy link
Member

It should be somewhere, although as mentioned - it is not recommended. It's better to use call wrappers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants