Redux saga for easy testing and extending

[mik639]

@joshwcomeau Hi Joshua, I'm continuing with tests and now trying to cover logic in middlewares. And I have a suggestion to use redux-saga instead of writing custom middleware for each case. redux-saga helps to write flat sync code, and it also has one more advantage it's pretty testable :) I provide an example with "dependency.middleware" in this pr

[superhawk610]

@mik639 I'm working on a solution for #33 right now and also came across redux-saga when looking for a way to queue action dispatches from within a reducer. Essentially, ADD_DEPENDENCY_START, UPDATE_DEPENDENCY_START, and DELETE_DEPENDENCY_START actions should do one of two things when dispatched:

1. If there are no other package.json-locking actions active for the current project, then take effect immediately.
2. If there are other package.json-locking actions active, add the action to a queue. Subsequent actions of the same type should be appended to the same entry in the queue so they can be bundleld into a single call to npm install or npm remove. Whenever a package.json-locking action completes, shift the next action item off the queue and run it

Does this sound like a good use case for redux-saga? My current solution is a simple middleware to allow for reducers to queue actions for dispatch once that reducer cycle has completed - here's the logic:

// queue.middleware.js
export default store => next => action => {
  let syncActivityFinished = false;
  let actionQueue = [];

  const flushQueue = () => {
    actionQueue.forEach(action => store.dispatch(action));
    actionQueue = [];
  };

  const queueDispatch = queuedAction => {
    actionQueue = [...actionQueue, queuedAction];

    if (syncActivityFinished) flushQueue();
  };

  const actionWithQueueDispatch = { ...action, queueDispatch };
  next(actionWithQueueDispatch);
  syncActivityFinished = true;
  flushQueue();
};

// queue.reducer.js
export default (state: State = initialState, action: Action) => {
  switch (action.type) {
    /* ... */
    case ADD_DEPENDENCY_FINISH:
    case ADD_DEPENDENCY_ERROR:
    case UPDATE_DEPENDENCY_FINISH:
    case UPDATE_DEPENDENCY_ERROR:
    case DELETE_DEPENDENCY_FINISH:
    case DELETE_DEPENDENCY_ERROR:
    case ADD_DEPENDENCIES_FINISH:
    case ADD_DEPENDENCIES_ERROR:
    case DELETE_DEPENDENCIES_FINISH:
    case DELETE_DEPENDENCIES_ERROR:
      return produce(state, draftState => {
        // Once a package.json-locking function completes (whether successfully
        // or not), fire the next action in the queue (if there are any).
        if (state.length > 0) {
          const nextAction = state.shift();
          const nextActionCreator = nextAction.action === 'install' ? addDependenciesStart : deleteDependenciesStart;

          action.queueDispatch(nextActionCreator(
            projectId: action.projectId,
            dependencies: nextAction.dependencies,
          ));
        }
      });
      
    default:
      return state;
  }
};

[mik639]

@superhawk610 yes, I think it's a good case for redux-saga. I have change a bit addDependency saga for matching you requirements.

export function* addDependency({ projectId, dependencyName, version }) {
  // isPackageJsonLock is selector that looking for "updating", "installing" and "deleting" statuses of dependencies
  const isLock = yield select(isPackageJsonLock, projectId);
  if (isLock) {
    // if package.json locked we add current task to queue, and cancel it.
    yield call(addToQueue, { projectId, dependencyName, version });
    yield cancel();
  }
  const projectPath = yield select(getPathForProjectId, projectId);
  try {
    yield call(installDependency, projectPath, dependencyName, version);
    const dependency = yield call(
      loadProjectDependency,
      projectPath,
      dependencyName
    );
    yield put(addDependencyFinish(projectId, dependency));
  } catch (err) {
    yield call([console, 'error'], 'Failed to install dependency', err);
    yield put(addDependencyError(projectId, dependencyName));
  }
}

then we can create handleQueue saga and subscribe it to *_FINISH, *_ERROR actions

[joshwcomeau]

Hey @mik639,

Ahh that's a great idea. I've used Redux Saga a bunch in the past, not sure why it didn't occur to me to use it here.

I suppose one reason against is that it's a bit less familiar to developers, so it might be harder for folks to contribute... but especially for the queue in #33, I can see it being really helpful.

So, here's the thing. I'm into the idea, but I'd like to convert all 3 of our middlewares to sagas if we're gonna move in that direction. Would you be willing to take that work on, @mik639?

---

@superhawk610 looks like Mikhail beat me to the reply, but yeah, I think Sagas would work well for the problem you're facing.

(One issue with the solution you propose is that redux reducers are meant to be side-effect-free; so they shouldn't be calling functions that affect the universe outside the reducer. Doing so would break the devtools, and potentially lead to some nasty bugs)

I'm not sure if Mikhail's solution really adds queue functionality, though (eg. it's not clear to me what addToQueue would do). I haven't worked with redux saga in a while (well over a year), so it's a bit rusty, but I think it could look something like this:

// We'd have a long-running saga that is started when the app loads, and listens for "ADD_DEPENDENCY_START"
export function* dependencies() {
  const queue = [];

  // Scary infinite loop works because Redux saga pauses on `take` until the action is seen
  while (true) {
    const action = yield take(ADD_DEPENDENCY_START);

    // At this point in time, either the queue is empty, or there's already one in progress.
    // We could either push this onto the queue, or consolidate it with an existing queue item
    if (canConsolidate) {
      const queuedItem = queue.pop();
      queue.push({ ...queuedItem, newItem });
    } else {
      queue.push(newItem);
    }
    
  }
}

The code as-written wouldn't work (one big problem is that we'd need 1 queue per projectId, but beyond that I'm pretty sure this code wouldn't handle multiple dispatches the way I expect?). Hopefully @mik639 sees where I'm going with this idea and can propose a proper fix in this direction. Essentially to have a queue inside a long-running saga that can listen for new actions and make a decision as to whether it should just be pushed into the queue, or consolidated with an item in the queue, based on the current status of the queue.

Also if there's a way to do this without totally changing the structure of the saga, that would be great 👍

[joshwcomeau]

This looks great!

Requesting changes because I think we should first update selectors to be (state, otherArgs) (my bad for not having them in that order to begin with!). I don't think we have too many selectors with additional args.

Also, I mentioned in the previous comment that I think we should commit to converting all the middlewares if we're gonna do this one, so we should make a decision on that before merging this.

[joshwcomeau]

Right, makes sense that we'd need to change the selectors to be state-first, for redux-saga select (it also just seems much more obvious, in retrospect, that this order makes more sense, since it matches how reducers work + allows for optional args afterwards).

I think we should try to update all selectors to follow this pattern, though, since I wouldn't want it to be inconsistent from reducer to reducer.

It might even make sense as a separate preliminary PR, to update all selectors (and their callsites) to be state-first... but I'm OK if we do it as part of this PR if that's too much git-juggling, haha.

[joshwcomeau]

These look great 👍

[joshwcomeau]

Tests look great!

Maybe just add a TODO comment for the other "branches"? Unless you plan on doing them as part of this PR, and were just waiting to see if I approved of sagas before spending the time?

[mik639]

@joshwcomeau, of course, I will rewrite all of them. Let's hold this pr open, I'll do it here.

[superhawk610]

@joshwcomeau Agreed, dispatches within a reducer function is certainly bad practice and Redux will specifically block it if you don't use some workaround like the middleware I posted. I'm relatively new to the Redux flow, so I'm still wrapping my head around best practices. I assume side effects are allowed in middleware, yes? And since saga functions as middleware, dispatching actions from there follows best practices.

I'll try to get up to speed on redux-saga this weekend and then refactor my preliminary stuff to conform to it so we'll all be on the same page.

[joshwcomeau]

@joshwcomeau, of course, I will rewrite all of them. Let's hold this pr open, I'll do it here.

Yay! Thanks @mik639

I assume side effects are allowed in middleware, yes? And since saga functions as middleware, dispatching actions from there follows best practices.

Right, middlewares are specifically built for side-effects (there's a great doc that details why middleware is useful for solving the problem of logging). And sagas are just a fancy middleware, really.

I'm keen to see how the solution will work! Maybe you and @mik639 could hack together on this (or, really, just keep each other in the loop so that your work is compatible?).

Might be helpful to go through exactly what the queue needs to do, so we're all on the same page:

• Only 1 dependency can be installed at 1 time, and we need a way to track the installation currently in progress (if any)
• If I try to install a dependency while one is in progress, it's added to a queue, and dealt with after the current installation is finished
• If there's already a queue, see if subsequent commands can be combined (eg. if the queue is ['npm i -s redux', 'npm i -s redux-devtools', 'npm i -s redux-devtools-log-monitor'], this can be combined into npm i -s redux redux-devtools redux-devtools-log-monitor)
• Each project should have its own queue (so if I have 4 projects in Guppy, Project 1's queue has no bearing on things I modify in Project 2)
• [possibly a stretch goal?] Same behaviour can exist for deleting dependencies, so that ['npm uninstall redux', 'npm install apollo', 'npm uninstall redux-devtools'] can be combined into [npm uninstall redux redux-devtools', 'npm install apollo']

[joshwcomeau]

Hey @mik639,

Looks like something funky happened (the same thing happened to @bennygenel, not sure why :/ I'm pretty sure I didn't do something dumb like force-push to master, but maybe I did?)

I think the way to fix it is to create a new branch from latest master, and cherry-pick the commits you need. I don't mind if you open a new PR, I can refer to this one for the discussion if needed.

Not sure how much Git experience you have, happy to share more info about the cherry-pick idea. Sorry for the hassle!

[mik639]

Hi @joshwcomeau
Yep, I think it's my fault with wrong rebase, I'm already created a new branch and working there, so we need this PR only for reference to the discussion

[joshwcomeau]

Closing this, as it was tackled in #117