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

Proposal: add subscribe to middlewareAPI #922

Closed
jonnyreeves opened this issue Oct 21, 2015 · 18 comments
Closed

Proposal: add subscribe to middlewareAPI #922

jonnyreeves opened this issue Oct 21, 2015 · 18 comments

Comments

@jonnyreeves
Copy link

Expand the existing middlewareAPI to include store.subscribe, ie:

var middlewareAPI = {
  getState: store.getState,
  dispatch: (action) => dispatch(action),
  subscribe: store.subscribe
};

Motivation

Stateful middleware implementations wish to observe the store to know when the store' state has been modified and trigger a side-effect; for example - OAuth middleware will detain CALL_API actions when it detects an expired OAuth Grant until the store has been updated with a new, valid Grant:

export function createMiddleware() {
  var detainedActions = [];

  return store => next => action => {
      if (!action[CALL_API]) {
        return next(action);
      }
      if (store.getState().oauthGrant.expired) {
        detainedActions.push(action);
        await obtainNewOauthGrant(store);
        redispatchDetainedActions(store.dispatch);
      } else {
        // ... sign the request.
    }
  }  

  async function obtainNewOauthGrant(store) {
    const unsubscribe = store.subscribe(() => {
      if (!store.getState().oauthGrant.expired) {
        unsubscribe();
        resolve();
      }
    });
    store.dispatch(obtainNewGrant());
  }

  function redispatchDetainedActions(dispatch) {
    // ... redispatch detained actions.
  }
}
jonnyreeves added a commit to jonnyreeves/redux that referenced this issue Oct 21, 2015
@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2015

Not sure about this. My current thinking is that if middleware needs to subscribe, it should be implemented as a store enhancer. This is how Redux DevTools and Redux Router works. We haven't documented this pattern yet, but we likely will in the future.

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2015

Example of a store enhancer:

const storeEnhancer = createStore => (reducer, initialState) => {
  const store = createStore(reducer, initialState);
  // do whatever you want with store object
  return store;
};

@gaearon
Copy link
Contributor

gaearon commented Oct 21, 2015

I agree with @acdlite on this. Middleware is meant to be “pass through” thing, anything more complicated should either be a store enhancer or just a function taking the store and calling subscribe on it for its purposes.

@gaearon gaearon closed this as completed Oct 21, 2015
@jonnyreeves
Copy link
Author

@acdlite, @gaearon, I trust your judgement on this; but I have been struggling with this particular detail for a while; there are already examples of middleware doing more than just "passing through" actions, for example, the API middleware in the real-world example.

From what I can see, Store enhancers do not have access to actions as they pass to reducers, so are therefore not in a position to intercept, sign and / or detain CALL_API actions when required by the app's business logic; likewise a function which wraps the store also has no way to access this flow - middleware appears to be the only way to tap into it.

Exposing the subscribe method in the middlewareAPI does not change the current signature for consumers and does not immediately result in bad-practice and is the only way to allow stateful middleware to work without resorting to importing store instances (which quickly leads to circular dependencies).

@johanneslumpe
Copy link
Contributor

@jonnyreeves The store enhancer has to pass the action to the reducer so yes, it is very much in the position to intercept actions.

You can add enhancers before your middleware and after and even do things like middleware -> enhancer -> middleware -> another enhancer. You are in full control. So if you wanted to prevent the CALL_API actions, you could just create a store enhancer and add it to the chain before your api call middleware.

@jonnyreeves
Copy link
Author

@johanneslumpe Thank you for the clarification; apologies for jumping to conclusions without reading the code.

@johanneslumpe
Copy link
Contributor

@jonnyreeves no worries :)

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2015

@jonnyreeves It may help to know that applyMiddleware() itself is a store enhancer :) https://github.com/rackt/redux/blob/master/src/utils/applyMiddleware.js

@jonnyreeves
Copy link
Author

Sorry to bring this up again; I believe the real problem I'm facing is that I need to trigger a side effect ( function) after an action has been reduced and the store's state updated.

It would appear that I am in need of a side-effects processor, similar to #569, as a result I knocked up a simple proof of concept side-effect processor which looks for meta.sideEffects on an action as it passes through to the reducer:

export default function sideEffectorExecutor(next) {
  return (reducer, initialState) => {
    const store = next(reducer, initialState);
    let pendingSideEffects;
    let unsubscribe;

    // custom `store.dispatch` implementation which records side-effects on the
    // action object and executes them on the next store update.
    const dispatch = (action) => {
      const { meta: { sideEffects } = {} } = action;

      if (Array.isArray(sideEffects)) {
        pendingSideEffects = sideEffects.concat();

        // one-shot subscription to invoke side-effects after the next store update.
        unsubscribe = store.subscribe(() => {
          unsubscribe();

          // process all side effects.
          pendingSideEffects.forEach(sideEffect => sideEffect());
          pendingSideEffects = null;
        });
      }

      // Pass the action through.
      store.dispatch(action);
    };

    return { ...store, dispatch }
  };
}

One issue I am facing with this approach is that I can't provide a meaningful store.dispatch function through to the side-effect (ie: sideEffect({ dispatch: store.dispatch })) as the supplied dispatch reference will not be decorated by another other store-reducers present on the 'final' store - I'm guessing there may be no solution to this problem as redux itself does not hold a reference to the final composed store instance, only the user's own code does.

@jlongster
Copy link

Not sure if this is relevant, but I have thought about this a lot. There are a few times where I really do need to "wait" on an action coming through the system at a later time. And I don't know what started the I/O, so I can't block on the promise (by composing action creators), all I know is that it's already started and I need to wait for it to finish.

I think I found a rather elegant solution to this, and I agree that we shouldn't (edit, I previously wrote "should") expose subscribe which is ripe for abuse. I created a middleware called "wait-service", and I call it a service because it's actually stateful. That's not a problem though because it's not state that I care about saving. You can see it here: https://github.com/jlongster/gecko-dev/blob/debugger-refactor-sources-4/devtools/client/shared/redux/middleware/wait-service.js

Basically, with this you can now dispatch an action that will fire something at a later time based on a predicate:

const { NAME: waitService } = require('wait-service.js');

dispatch({
  type: waitService,
  predicate: action => (action.type === "ADD_ITEM" &&
                        action.status === "done"),
  run: (dispatch, getState, action) => {
    // dispatch and getState are what you expect. `action` is the
    // action that fulfilled the predicate.
  }
});

I have found that I've been to solve some really complex async scenarious with this simple service that has a little state: a list of waiting actions that check each action coming through the system.

(I've been meaning to write a blog post about this)

@jlongster
Copy link

I may have mis-read the OP; looks like you just need the dispatch function. You can get this by receiving it in the middleware when its instantiated; check the wait-service.js signature.

@jonnyreeves
Copy link
Author

@jlongster, nope - you're spot on. I have middleware that contains state (detained actions) and I want to re-dispatch those actions when a predicate is met on the store.

The later request for dispatch is in response to using a store enhancer to try and achieve the same effect.

@gaearon
Copy link
Contributor

gaearon commented Oct 22, 2015

I have middleware that contains state (detained actions) and I want to re-dispatch those actions when a predicate is met on the store.

Now that I think of it.. The only way a store may change is by dispatch()ing an action. So you don't need to subscribe()—just add a predicate check right after you call the next() in the middleware, and if it's successful, dispatch the detained actions too.

No?

@jonnyreeves
Copy link
Author

@gaearon but there is no guarantee that the action will hit the reducer after the next() call; what if another piece of middleware / store enhancer detains the action, defers it, etc?

This brings me back to why I wanted to subscribe to the store in the middleware, to know, for sure, that the store is in the correct state to proceed.

@gaearon
Copy link
Contributor

gaearon commented Oct 22, 2015

but there is no guarantee that the action will hit the reducer after the next() call; what if another piece of middleware / store enhancer detains the action, defers it, etc?

If it doesn't reach the reducer, the state wouldn't get updated. I still don't understand the difference between “subscribing with a predicate” and “putting an action into an array, and next time an action is dispatched and the predicate is fulfilled, dispatch actions from array too.”

@jonnyreeves
Copy link
Author

If it doesn't reach the reducer, the state wouldn't get updated.

This is the crux of the problem, the following code can not be relied upon:

// This is the current state of the store.
assert.equal(getState().state, 'old_state');

// This action *should* update the 'state' property on the store.
dispatch(updateState('new_state'));

// So this assertion *should* be fine as the `updateState` action will have been reduced.
assert.equal(getState().state, 'new_state');

However, as per the redux documentation:

middleware is potentially asynchronous

The action could well be delayed and the assertion above could fail. Currently, middleware has no way to observe the store, and therefore no way to know, for certain, when a given predicate can be met.

I've created a github project which highlightes the issue I'm facing: https://github.com/jonnyreeves/redux-issue-922. The main actor is the auth-middleware. This is stateful, async middleware which will detain certain actions if the grant in the redux store has expired. In the real implementation, the detained actions are replayed once a valid grant has been obtained.

If you npm install and then npm test you will see a failing test-case that demonstrates this, and the code I would like to write to get it to pass.

Thanks.

@gaearon
Copy link
Contributor

gaearon commented Oct 23, 2015

Yes, but then you should just warn the user to put middleware at the end of the middleware chain.
That is, enforce it being synchronous. This is similar to how redux-logger wants to be last so it can log the next state.

Sure, it's not fool proof, but this is the best mechanism we got right now. Otherwise, feel free to use a store enhancer as described above.

@jonnyreeves
Copy link
Author

Thanks for your input @gaearon.

I have solved my particular problem by creating a custom store enhancer: redux-action-side-effects which invokes an action's side effects after the action has been reduced. My authentication middleware then attaches a side-effect to the auth action as it passes through.

I personally feel there is still a case for adding subscribe to the middleware API however I can understand the desire to keep the API's surface area as small as possible.

Thanks again for your hard work on redux. 🍻

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

5 participants