Solution feedback: redux-saga-jaysoo

[tomsdev]

Hi, I looked at @jaysoo solution that's using redux-saga and would like to share my main concern:

Every component that contains a RandomGif component (for example RandomGifList) duplicates RandomGif actions. Isn't it possible to reuse RandomGif actions instead of duplicating them?

The consequences of this duplication of actions are visible in:

• RandomGifList/RandomGifPair reducers are almost reimplementing RandomGif reducer, each action one by one.
• RandomGifList/RandomGifPair sagas look very similar to RandomGif saga.
• In main/index.js, in order to react to the NEW_GIF action it needs to subscribe to all its duplicates:

yield race(
      [ take(randomGifList.actions.NEW_GIF)
      , take(randomGif.actions.NEW_GIF)
      , take(randomGifPair.actions.NEW_GIF)
      , take(randomGifPairOfPair.actions.NEW_GIF)
      ]
    )

[jaysoo]

@tomsdev Hi, thanks for the feedback. The problem with reusing action creators from the original randomGif module everywhere is that there wouldn't be a way reduce properly based solely on actions of type:

type NewGifAction = {
  type: string;
  payload: string;
}

You will have to introduce an unique identifier for each NewGifAction dispatched so that the right GIF is updated in the state atom.

It's definitely doable, but will probably require randomGif module to be refactored. The solution I've provided leaves each submodule untouched, but does require more boilerplate.

I'd love to see any improvements made to be original solution. Especially reducing boilerplate, but still keeping modules as decoupled as possible.

[tomsdev]

I don't think you'd need to change RandomGif, if you look at @jarvisaoieong solution you'll see that he is able to reuse the actions from RandomGif even though RandomGif is a simple reusable component like you have.

He does it by wrapping the dispatched RandomGif action inside a MODIFY action in RandomGifList. Then, in RandomGifList reducer, on action MODIFY, he unwrap the inner action and pass it to the RandomGif reducer.

Do you see any reason why this would not work for your solution?

[jaysoo]

@tomsdev The difference here is that redux-loop contains the effect to the local reducer, whereas redux-saga acts on a global store level. This means that an effect of MODIFY on randomGifPair reducer will eventually reduce down to NEW_GIF action, which will reduce to a new state (but only within randomGifPair).

Because sagas coordinate actions on the store level, you cannot dispatch NEW_GIF action only to randomGifPair module, since the store dispatch will dispatch to all combined reducers.

So unfortunately, I don't think there is a way to simplify the main saga with the current setup. You could perhaps watch for RUN_TASK instead, and figure out if the task is fetchRandomGif, but I think it'll require refactoring of the randomGif module.

[tomsdev]

I don't understand your response entirely but I do think there are ways that a Saga acting on the global store could match a "deep" randomGif.actions.NEW_GIF action even if it is wrapped/composed by RandomGifList.

[jaysoo]

The problem isn't from the saga side, it's actually the reducers. If there is only one shared NEW_GIF action, then the reducers will not be able to match their own actions properly. Even if you dispatch NEW_GIF with some meta properties, it still won't prevent the original randomGif module from updating its state.

For example,

const randomGifReducer = (state, action) => {
  switch (action.type) {
    case 'NEW_GIF':
      // Should I update my own state, or is this
     // action meant for another module's reducer?
    default:
      return state
  }
}

const randomGifPairReducer = (state, action) => {
  switch (action.type) {
    case 'NEW_GIF':
      // Should I update my own state, or is this
     // action meant for another module's reducer?
    default:
      return state
  }
}

The solution using redux-loop doesn't have this problem because reduced effects are local to the reducer currently performing the update. In this case, a MODIFY_RANDOMGIFLIST action will only match the randomGifList reducer (every other reducer does a no-op), which reduces to a local NEW_GIF action (at which point other reducers already ignored this action).

For sagas, the solution is constrained to global effects in the system, so the only way to differentiate them is either:

1. Through the use of meta properties on action that all reduces of same type understand (e.g. add an ID for each gif). This is doable, but will need randomGif to act on new properties.
2. By dispatching differently namespaced actions for each module (which is what I've opted to do).

Also, the redux-loop solution does some extra work when combining reducers into the root reducer. This higher-order reducer is analogous to combining the sagas from each GIF module. With sagas, the reducer (update) and sagas (effects) are combined separately, whereas in the redux-loop example they are combined together (update + effects).

[tomsdev]

I understand, the idea is not to add some meta properties but to compose/wrap actions like in the example I mentioned before or directly in the action type (e.g. 'NEW_GIF' action type becomes 'GifList.0.RandomGif.NEW_GIF'). This way the randomGifReducer will just ignore the action coming from GifList.

The saga in main/index.js will still need to specify the different patterns it wants to match (e.g. 'NEW_GIF', 'GifList.[].RandomGif.NEW_GIF', etc.).

The gifListReducer will directly pass actions to randomGifReducer when the action type match the pattern'GifList.[].RandomGif.*'.

You can see an implementation of action type pattern matching in the redux-elm example.

[jaysoo]

I see, that's an interesting approach. I'm hesitant to use routing in action types, not because it's bad but because it requires more frameworking around it. If someone wants to extract action type routing into a mini-library I'd be willing to take a look. ;)

For a more low-cost solution (assuming we're trying to reduce boilerplate), we can remove most of the sagas from the application. Instead, we'll have a saga to watch for RUN_TASK, and instead of providing mapped Tasks (using lift), we can provide a task descriptor that specifies the functions that arguments should be piped through.

Here's how we can create and pipe tasks.

Here's the base GIF task and how it is run

Here is the resulting task watch + counter update.

[tomsdev]

If someone wants to extract action type routing into a mini-library I'd be willing to take a look.

That's an interesting idea, if I start using this in a project I will extract that part as a library.

For a more low-cost solution (assuming we're trying to reduce boilerplate), we can remove most of the sagas from the application. Instead, we'll have a saga to watch for RUN_TASK

Thanks for trying to improve that, it does remove the duplication of sagas but NEW_GIF actions and reducer are still duplicated. It is definitely an interesting approach but the challenge specify:

The counter value should be incremented everytime a NEW_GIF action is fired from any NewGif component, no matter the nesting, but the incrementation amount is not fixed.

However, now in your solution the saga increments the counter not in response to a NEW_GIF action that has been dispatched but in response to randomGif.tasks.fetchRandomGif which is kind of different.

I can imagine a NEW_GIF action that wouldn't need fetching because it is already in the cache and then randomGif.tasks.fetchRandomGif wouldn't be executed so the saga wouldn't increment the counter.

[jaysoo]

You just gave me a good idea in regards to pattern matching. I don't know how well it'll work yet, but if you think of actions as recursive types, then using catamorphism rather than pattern matching could more powerful and reusable.

I think I'll leave the plain saga implementation as is, but I encourage you improve upon it if you want. :)

[jaysoo]

Of course now that I think about it, catamorphism would be harder to implement because the action types in Redux are just strings. Pattern matching could be the easier solution. :)

[tomkis]

@jaysoo indeed good job and this part is truly perfect decoupling.

However, what your submission certainly does not meet is second criterium and @tomsdev has already mentioned that, since there's too much code duplication. For example logic of GifViewer is duplicated in all of their super components (GifViewerPair...)

The aim of decoupling the components is that a team can take ownership of each component. Then another team is responsible of making all the components work nicely together, and you already have split the work into 4 teams.

And I believe you have already answered why this is not possible by saying:

The problem isn't from the saga side, it's actually the reducers. If there is only one shared NEW_GIF action, then the reducers will not be able to match their own actions properly. Even if you dispatch NEW_GIF with some meta properties, it still won't prevent the original randomGif module from updating its state.

And @tomsdev is definitely right by saying:

The saga in main/index.js will still need to specify the different patterns it wants to match (e.g. 'NEW_GIF', 'GifList.[].RandomGif.NEW_GIF', etc.).

Which is... frankly, very difficult to implement, since you'd need to be able to instantiate Sagas.

tldr; Using Sagas for inter-component communication sounds promising but it's very difficult to compose them so that we'd get truly encapsulated scalable solution.