salsita / prism

React / Redux action composition made simple http://salsita.github.io/prism/
495 stars 24 forks source link

Using takeEvery executes multiple times for a single action #37

Closed eliperelman closed 8 years ago

eliperelman commented 8 years ago

Trying to pinpoint the issue, but it seems that every time I try to use takeEvery with a simple call, the action is executed multiple times, in my case, it seems that the console.log is run 4 times. My reduced case:

function* rootSaga() {
  yield* takeEvery(SIGN_IN, function*() {
    yield call(() => console.log('do something...'));
  });
}

Running this in a loop instead seems to have the same issue:

function* signIn() {
  while (true) {
    yield take(SIGN_IN);
    yield call(() => console.log('do something'));
  }
}

function* rootSaga() {
  yield fork(signIn);
}

In my view I have a button that is bound to a signIn function via:

const signIn = () => dispatch({ type: 'SIGN_IN' });

Adding a console.log to this method only logs a single time, so the event/dispatch isn't happening multiple times:

const signIn = () => {
  console.log('Signing in...'); // this only logs once
  dispatch({ type: 'SIGN_IN' });
};

Reducer, for clarity:

import { Updater } from 'redux-elm';
import { takeEvery, delay } from 'redux-saga';
import { take, fork, call, put } from 'redux-saga/effects';

const SIGN_IN = 'SIGN_IN';

function* signIn() {
  while (true) {
    yield take(SIGN_IN);
    yield call(() => console.log('take some action...'));
  }
}

function* listen() {
  yield [
    fork(signIn)
  ];
}

export default new Updater({}, listen).toReducer();

I'm using a bit of changes to the boilerplate, so I'll post that as well shortly.

tomkis commented 8 years ago

Just tried that in redux-elm-skeleton and it seems to work just fine:

import { Updater } from 'redux-elm';
import { call, take, fork } from 'redux-saga/effects';

const initialModel = {
  greeted: false
};

function* signIn() {
  while (true) {
    yield take('SayHi');
    yield call(() => console.log('take some action...'));
  }
}

function* listen() {
  yield [
    fork(signIn)
  ];
}

export default new Updater({}, listen)
  // .case('SayHi', model => ({ ...model, greeted :true }))
  .toReducer();
eliperelman commented 8 years ago

Could be my boilerplate then, I'll post it.

eliperelman commented 8 years ago

Boilerplate:

index.js (entry point) ``` js import create from './init'; const start = create(document.getElementById('root')); if (module.hot) { module.hot.accept(start); } start(); ```
init.js ``` js import React from 'react'; import { render } from 'react-dom'; import { createStore, compose, combineReducers } from 'redux'; import reduxElm from 'redux-elm'; import { browserHistory } from 'react-router'; import { syncHistoryWithStore } from 'react-router-redux'; import routes from './routing'; import reducers from './reducers'; import Root, { instrument, persist } from './components/Root'; const storeFactory = compose(reduxElm, instrument, persist)(createStore); let store; export default (domNode) => () => { if (!store) { store = storeFactory(combineReducers(reducers)); } else { store.replaceReducer(combineReducers(reducers)); } const history = syncHistoryWithStore(browserHistory, store); render( , domNode ); }; ```
routing.js ``` js import R from 'ramda'; import React from 'react'; import { connect as _connect } from 'react-redux'; import { forwardTo } from 'redux-elm'; import { basename, dirname } from 'path'; /** * getModelKey :: String -> String */ const getModelKey = R.converge(R.concat, [ R.pipe(R.head, R.toLower), R.tail ]); /** * extractPageName :: String -> String */ const extractPageName = R.pipe(dirname, basename); /** * connect :: Component -> String -> [String] -> Component */ const connect = (View, modelKey, ...nesting) => { const factory = _connect(state => ({ model: state[modelKey] })); return factory(props => ( modelKey === 'app' ? : )); }; /** * getActionKey :: String -> String */ const getActionKey = R.pipe( R.split(/(?=[A-Z])/), R.map(R.toUpper), R.join('_') ); const req = require.context(`./pages`, true, /index\.js/igm); export const pages = req .keys() .reduce((childRoutes, f) => { const name = extractPageName(f); const value = req(f); childRoutes[name] = R.merge(value, { component: connect(value.default, getModelKey(name), getActionKey(name)) }); return childRoutes; }, {}); export const { Home, NotFound } = pages; /** * toRoutes :: Object -> List Object */ const toRoutes = R.pipe( R.dissoc('Home'), R.dissoc('NotFound'), R.values, R.append(NotFound) ); export default { path: '/', component: connect(require('./App').default, 'app', 'APP'), indexRoute: Home, childRoutes: toRoutes(pages) }; ```
reducers.js ``` js import R from 'ramda'; import { routerReducer } from 'react-router-redux'; import { basename, dirname } from 'path'; /** * getModelKey :: String -> String */ const getModelKey = R.converge(R.concat, [ R.pipe(R.head, R.toLower), R.tail ]); /** * getReducerName :: String -> String */ const getReducerName = R.pipe(dirname, basename, getModelKey); const req = require.context(__dirname, true, /updater\.js/igm); const reducers = req .keys() .reduce((reducers, f) => { reducers[getReducerName(f)] = req(f).default; return reducers; }, {}); export default { routing: routerReducer, ...reducers }; ```

So, I'm doing a bit of work to automate the connection of routes and reducers, so I may have missed something that is causing the error.

eliperelman commented 8 years ago

OK, doing some more digging it appears that I get a console.log('take some action...') once for every reducer that uses a saga. For example, let's say I have the following reducers set up at app init:

import reducers from './reducers';
const rootReducer = combineReducers(reducers);

console.log(reducers);
/*
{
  app: function() {...},
  home: function() {...},
  login: function() {...},
  routing: function() {...},
  somePage: function() {...}
}
*/

Let's say app, login, and somePage are using a reducer with a saga:

export new Updater(init, saga).toReducer();

When the take(SIGN_IN) occurs, I will get 3 console.logs. If I go into the login reducer and remove the saga:

export new Updater(init).toReducer();

Then I only get 2 console.logs.

eliperelman commented 8 years ago

I'll try and get this into a reduced repo to debug on.

eliperelman commented 8 years ago

OK, I have a mostly reduced repo at: https://github.com/eliperelman/redux-elm-37

tomkis commented 8 years ago

@eliperelman So the issue is that your Updater hierarchy does not form tree hierarchy and therefore the Sagas as re-instantiated. I am currently working on redux-elm next version which will warn user to console.

screen shot 2016-07-21 at 10 10 07

Anyway the idea is that your App/updater should handle all the actions and proxy them to corresponding pages. Therefore you wouldn't use combineReducers but do the composition manually in App/updater. Please have a look at our boilerplate because we solved the routing exactly that way (routing definition)

eliperelman commented 8 years ago

Perfect, thanks so much for the help! I've incorporated your implementation of the boilerplate with my own dynamic page and routing connect, and it seems to work OK. Thanks again!