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

Making Found react to Farce push/replace actions #80

Closed
ravicious opened this issue May 8, 2017 · 15 comments
Closed

Making Found react to Farce push/replace actions #80

ravicious opened this issue May 8, 2017 · 15 comments

Comments

@ravicious
Copy link

ravicious commented May 8, 2017

I set up Found using the createConnectedRouter way. When I dispatch push or replace actions from Farce, I see that they're dispatched, but the reducer state doesn't change and as a result I'm not redirected to the target location.

Is there anything obvious that I missed in the config? Basically I need a way to redirect the user from within my action creators.

@ravicious
Copy link
Author

What's weird is that when I dispatch @@farce/PUSH "by hand" from the Redux devtool, the router reacts properly and I'm redirected.

My store setup looks like this:

const store = createStore(
  reducers,
  composeEnhancers(
    createHistoryEnhancer({
      protocol: new BrowserProtocol(),
      middlewares: [queryMiddleware],
    }),
    createMatchEnhancer(new Matcher(routeConfig)),
    // The saga middleware has to be specified after the Found/Farce middlewares here,
    // because some sagas may listen to Found/Farce actions and they need to be notified about them.
    // If the saga middleware is specified before Found/Farce middlewares,
    // it doesn't receive the `UPDATE_MATCH` action.
    applyMiddleware(thunk, sagaMiddleware)
  )
)

@ravicious
Copy link
Author

Some additional findings:

When applyMiddleware(…) is passed after Found/Farce middlewares, I receive @@found/UPDATE_MATCH in my sagas, but dispatching Farce actions from my action creators has no effect.

When applyMiddleware(…) is passed before Found/Farce middlewares, I don't receive @@found/UPDATE_MATCH, but I do receive @@farce/INIT, @@farce/PUSH, @@farce/RESOLVE_MATCH in sagas and dispatching Farce actions works. However, this solution is not optimal for me, because I'd like to listen to a single event which includes location pathname in the payload and that is dispatched on initialization and subsequent navigation.

@ravicious
Copy link
Author

Update: I fixed this by reordering my middlewares as such:

const store = createStore(
  reducers,
  composeEnhancers(
    applyMiddleware(thunk),
    createHistoryEnhancer({
      protocol: new BrowserProtocol(),
      middlewares: [queryMiddleware],
    }),
    createMatchEnhancer(new Matcher(routeConfig)),
    // The saga middleware has to be specified after the Found/Farce middlewares here,
    // because some sagas may listen to Found/Farce actions and they need to be notified about them.
    applyMiddleware(sagaMiddleware)
  )
)

This way Farce actions work and Saga receives UPDATE_MATCH. It seems like it may be a redux-thunk issue, as I found other people with similar problems: reduxjs/redux-thunk#9 acdlite/redux-router#87

Closing this, as I don't believe it's Found's problem.

@ravicious
Copy link
Author

Actually this doesn't solve the problem, sorry for the confusion. :(

Thunk needs to be specified after saga in order for the saga to be able to dispatch thunks, see reduxjs/redux-thunk#134 (comment).

So I'm back to square one, maybe with the order of middlewares in applyMiddleware being different (first saga, then thunk).

I'm out of ideas. 😣

@ravicious ravicious reopened this May 8, 2017
@taion
Copy link
Contributor

taion commented May 8, 2017

Shouldn't you be putting thunk after saga? If you add logging, can you tell where the action you want to see gets swallowed? Also, you can always just compose in the thunk middleware both before and after the router middlewares (though I don't think you should need to do that).

@ravicious
Copy link
Author

Alright, I added console.log to middlewares. This is how the store looks like:

const store = createStore(
  reducers,
  composeEnhancers(
    createHistoryEnhancer({
      protocol: new BrowserProtocol(),
      middlewares: [queryMiddleware],
    }),
    createMatchEnhancer(new Matcher(routeConfig)),
    applyMiddleware(sagaMiddleware, thunk)
  )
)

When I'm dispatching the push action from a saga, in the logs I can see that it propagates only through saga and thunk middleware.

When I dispatch the push action from the Redux devtool, it's consumed by ensureLocationMiddleware of farce and doesn't propagate to other middlewares.

The working setup I had with react-router-redux looked like this:

const store = createStore(
  reducers,
  composeEnhancers(applyMiddleware(thunk, routerMiddleware(browserHistory), sagaMiddleware))
)

@ravicious
Copy link
Author

To shed some more light: I have a saga that listens to @@found/UPDATE_MATCH and conditionally dispatches @@farce/PUSH.

@ravicious
Copy link
Author

ravicious commented May 8, 2017

If I put applyMiddleware in front of Farce/Found enhancers, then @@farce/INIT passes through applyMiddleware, finally goes to the createMatchEnhancer middleware which consumes it and passes @@found/UPDATE_MATCH, but saga can't consume this action since it doesn't even see it.

I don't know if it'd be feasible to change this behavior of Found so that it'd "redispatch" UPDATE_MATCH instead of passing it to the next middleware.

Update: From what I understand, this is not possible, as the enhancer may only pass the action to the next middleware. To "redispatch" the action, the match enhancer would have to be a part of applyMiddleware, like react-router-redux.

@ravicious
Copy link
Author

I found some other libs with exactly the same problem: FormidableLabs/redux-little-router#36 (comment). This comment links to a bunch of other issues, most notably reduxjs/redux#1051.

From my PoV it seems like the only solution for Found and Farce would be to sit in the same place in chain as other libraries (applyMiddleware), but I don't know if it's even possible to implement Found/Farce enhancers as middlewares.

For now I'll use this dirty workaround. It passes the original dispatch to redux-thunk, which allows me to dispatch actions from thunks that will be received by Found/Farce.

const extraArguments = {}

const store = createStore(
  reducers,
  composeEnhancers(
    createHistoryEnhancer({
      protocol: new BrowserProtocol(),
      middlewares: [queryMiddleware],
    }),
    createMatchEnhancer(new Matcher(routeConfig)),
    applyMiddleware(sagaMiddleware, thunk.withExtraArgument(extraArguments))
  )
)

extraArguments.dispatchForRouting = store.dispatch

@taion
Copy link
Contributor

taion commented May 11, 2017

So you're saying that, if this were a middleware, you could call dispatch in your async middlewares, and those would re-dispatch through the Farce/Found stuff?

The reason these are store enhancers is because I need to expose getter methods that touch things the middlewares close over. This could be accomplished with dispatch (and looking at the return value), but that runs the risk of hitting user error in case some middlewares in the chain don't return the value of next().

Couldn't this be solved with something like a composeEnhancersWithDispatch or applyEnhancerChain (or something with a better name) that exposes applyMiddleware-like semantics where the dispatch given to each enhancer becomes the root dispatch?

@taion
Copy link
Contributor

taion commented May 11, 2017

Or that wouldn't quite work. But maybe something like:

const store = createStore(
  reducers,
  composeWithMiddleware(
    createHistoryEnhancer({
      protocol: new BrowserProtocol(),
      middlewares: [queryMiddleware],
    }),
    createMatchEnhancer(new Matcher(routeConfig)),
    { middleware: sagaMiddleware },
    { middleware: thunk },
  ),
);

I'm spitballing on the API here... but this should work.

@taion
Copy link
Contributor

taion commented May 11, 2017

Check this out:

import { applyMiddleware, compose, createStore } from 'redux';

const middleware1 = store => next => (action) => {
  console.log(1, action);
  return next(action);
}

const middleware2 = store => next => (action) => {
  console.log(2, action);
  return next(action);
}

const middleware3 = ({ dispatch }) => next => (action) => {
  console.log(3, action);
  return action.type === 'foo' ? dispatch({ type: 'bar' }) : next(action);
}

const store1 = createStore(() => {}, compose(
  applyMiddleware(middleware1),
  applyMiddleware(middleware2, middleware3),
));

console.log('store1')
store1.dispatch({ type: 'foo' });

function composeWithMiddleware(...enhancers) {
  enhancers.reverse();

  return createBaseStore => (...args) => {
    let createStore = createBaseStore;
    let store;

    const rootDispatch = action => store.dispatch(action);

    enhancers.forEach((enhancer) => {
      if (typeof enhancer === 'function') {
        createStore = enhancer(createStore);
        return;
      }

      const createNestedStore = createStore;

      let middlewares = enhancer.middleware;
      if (!Array.isArray(middlewares)) {
        middlewares = [middlewares];
      }

      createStore = (...nestedArgs) => {
        const nestedStore = createNestedStore(...nestedArgs);
        const middlewareApi = {
          getState: nestedStore.getState,
          dispatch: rootDispatch,
        };

        const chain = middlewares.map(middleware => middleware(middlewareApi));

        return {
          ...nestedStore,
          dispatch: compose(...chain)(nestedStore.dispatch),
        };
      }
    });

    store = createStore(...args);
    return store;
  }
}

const store2 = createStore(() => {}, composeWithMiddleware(
  applyMiddleware(middleware1),
  { middleware: [middleware2, middleware3] },
));

console.log('store2')
store2.dispatch({ type: 'foo' });

@ravicious
Copy link
Author

Couldn't this be solved with something like a composeEnhancersWithDispatch or applyEnhancerChain (or something with a better name) that exposes applyMiddleware-like semantics where the dispatch given to each enhancer becomes the root dispatch?

Did you have the chance to read through reduxjs/redux#1051? As far as I understand, using such a custom compose version wouldn't work with DevTools. Also, the current semantics of dispatch inside enhancers is a feature not a bug, so I wouldn't expect that to change.

It seems that the only solution would be to implement a middleware rather than an enhancer.

The reason these are store enhancers is because I need to expose getter methods that touch things the middlewares close over. (…)

Are you referring to this piece of code in createMatchEnhancer? What other part of the codebase expects that matcher to be there?

@taion
Copy link
Contributor

taion commented May 12, 2017

It gets used by the rest of Found to pass methods around from the store.

Take a look at the code snippet I have above. There's no necessary or technical reason why middlewares can't dispatch upstream to enhancers.

You can also see that the core of both Farce and Found does sit in middlewares, so we could rejigger the API to expose the middlewares more directly. However, I don't think I can get around the requirement of having to stick some methods onto the store, to allow e.g. the pattern where you just set up a <Provider> and pass everything around on the store object, without exposing users to potential bugs in case they install a middleware or enhancer before the Farce/Found middlewares that make actions async, or something.

@taion
Copy link
Contributor

taion commented Jun 19, 2017

Going to close this out for now – this seems like a Redux limitation with a few decent enough workarounds, and I don't want to constrain the API by making these be strictly middlewares.

@taion taion closed this as completed Jun 19, 2017
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

2 participants