handleActions returning defaultState even when state passed is not undefined

[amit1911]

import { handleActions } from 'redux-actions';

const getInitialState = () => {
    return ['red'];
};

const reducer = handleActions({
    test: (state, action) => {
        return action.payload.a ? undefined : ['yellow'];
    },
    test2: (state) => {
        return ['1'];
    }
}, getInitialState());

const reducer2 = handleActions({
    test: (state, action) => {
        return action.payload.a ? undefined : ['yellow'];
    }
}, getInitialState());

console.log(reducer(['green'], {
    type: 'test',
    payload: {
        a: 1
    }
}));
// ['red']

console.log(reducer2(['green'], {
    type: 'test',
    payload: {
        a: 1
    }
}));
// undefined

The first console statement returns the defaultState which is ['red'] whereas the second console statement returns undefined even though they both respond to the same action and have same action handling logic. Can someone tell me why is this happening?

[yangmillstheory]

I can't reproduce this. Here's the (full) code snippet:

const { handleActions } = require('redux-actions')

const getInitialState = () => {
    return ['red'];
};

const reducer = handleActions({
    test: (state, action) => {
        return action.payload.a ? undefined : ['yellow'];
    },
    test2: (state) => {
        return ['1'];
    }
}, getInitialState());

const reducer2 = handleActions({
    test: (state, action) => {
        return action.payload.a ? undefined : ['yellow'];
    }
}, getInitialState());

console.log(reducer(['green'], {
    type: 'test',
    payload: {
        a: 1
    }
}));
// ['red']

console.log(reducer2(['green'], {
    type: 'test',
    payload: {
        a: 1
    }
}));

And though I don't think it matters:

> node -v
v7.4.0

[amit1911]

That's weird. I can easily reproduce this on http://requirebin.com/ What output are you getting? Can you reproduce this on requirebin?

[yangmillstheory]

example node test.js
undefined
undefined

Expected in both cases since action.payload.a is truthy (although I'm not sure why you're returning undefined; state shouldn't be undefined).

If I changed the undefined return value on requirebin, I get correct output:

Same as in the script:

example node test.js
foo
foo

Looks like a bug on their end.

[amit1911]

I am not actually returning undefined. This was a bug I found in the code and while I was fixing it, I thought that it should have broken my application but somehow it didn't. Then I investigated and found this issue.

You mentioned that you get correct result when you use some valid value instead of undefined. Does this mean you can reproduce the bug on requirebin as well? I am also getting correct results if I use a valid value instead of undefined but this bug is only for undefined case and since this is happening in my application as well I think there might be an issue worth investigating here.

[yangmillstheory]

In node, get the correct result either way. I suspect it'll be the same if I use Babel. I think the issue is with requirebin.

[amit1911]

I am using Babel and still facing this issue and I don't think this should be an issue with requirebin either as they are just loading the module and it seems to be working correctly for all the other cases. This is so strange. Maybe we should re-open this issue and see if someone else has any comments on this as well.

[yangmillstheory]

Can you post a repo with a minimal working example and test in Babel?

[amit1911]

I have uploaded the project to http://www.filedropper.com/redux-actions-bug-197. I used the google starter kit for this https://developers.google.com/web/tools/starter-kit/. Follow the steps to reproduce the bug:-

1. Download the project at the above link.
2. Run npm install
3. Install browserify globally. cd into app/scripts/ directory and run browserify index.js -o bundle.js
4. Start server by running gulp serve. The project should open in your browser window. Open console and check the output.

[amit1911]

@yangmillstheory were you able to reproduce the issue by following the above steps?

[yangmillstheory]

Sorry, let me try now.

[yangmillstheory]

Your link above is broken by the way.

I also wouldn't recommend having users install dependencies globally. I don't need/want a global copy of browserify.

[amit1911]

looks like Filedropper only keeps the link active for some time. I am having trouble uploading my zip file to github directly. Lemme try something else.

[yangmillstheory]

No - the markdown link syntax you used is incorrect.

---

So, I can reproduce the issue, but again, I don't think this is a bug in this library. Something strange is going on in the starter kit you're using (gulpfile.babel.js is 256 lines long, e.g.).

You might want to browse the tests and source code here for assurance. https://github.com/acdlite/redux-actions/blob/master/src/__tests__/handleActions-test.js#L78 would be a good starting point.

I would suggest coming up with the most minimal example you can, like I did above, to try to debug your issue. I'd rather not step through all that transpiled code right now. Good luck!

[amit1911]

sorry i cannot see your example above; some markdown issue. The above test case only guarantees that if the initial state passed to the reducer is undefined then it should return the default state...but there is no test case that guarantees that if reducer returns undefined then it should be left untouched and not return initialState. i'll try to verify this by adding a test case for this rather than creating a full project which i agree is too complex for debugging this simple issue.

[amit1911]

Just verified the bug by adding the following testcases

import { expect } from 'chai';
import { handleActions, createAction } from '../';

describe('issue-197', () => {
    const defaultState = ['red'];

    it('A', () => {
        const reducer = handleActions({
            test: (state, action) => {
                return action.payload.a ? undefined : ['yellow'];
            },
            test2: (state) => {
                return ['1'];
            }
        }, defaultState);

        expect(reducer(['green'], { type: 'test', payload: { a: 1 } })).to.deep.equal(undefined);
    });

    it('B', () => {
        const reducer = handleActions({
            test: (state, action) => {
                return action.payload.a ? undefined : ['yellow'];
            }
        }, defaultState);

        expect(reducer(['green'], { type: 'test', payload: { a: 1 } })).to.deep.equal(undefined);
    });
});

The first test cases passes and the second one fails whereas both should have passed

[yangmillstheory]

Great! I stand corrected. What do the passing test cases look like?

I'm not sure what the behavior should be in this case. The signature of a reducer is (state, action) => nextState, and I'm not sure we should support reducers returning undefined; I'm doing research on that right now.

[yangmillstheory]

Returning undefined doesn't seem to be sanctioned behavior: https://github.com/reactjs/redux/blob/920e72e9921e4e9157494e841f526021105a2c03/src/combineReducers.js#L12

[amit1911]

I agree with the fact that returning undefined isn't correct but that is not the only issue here. Another issue is that the two reducers above return different results. Both should either return undefined or both should return defaultState. Either both should fail or both should pass.

[yangmillstheory]

Care to submit a pull request?

[amit1911]

okay I'll try to work on this during this weekend. I think we should open this issue now so that other people can offer help.

[amit1911]

The issue is not with this library, it's actually because of reduce-reducers which is being used by handleActions. reduce-reducers reduces multiple reducers into one reducer by the following code

export default function reduceReducers(...reducers) {
  return (previous, current) =>
    reducers.reduce(
      (p, r) => r(p, current),
      previous
    );
}

So the reason why test case A fails is because since the first reducer inside handleActions returns undefined, the second reducer will get undefined as the initial state and will return the default state. But if we push the test reducer to the end inside handleActions, then it will receive the actual state and will return undefined. So the following test case will pass.

it('A', () => {
        const reducer = handleActions({
            test2: (state) => {
                return ['1'];
            },

            // add this at the end
            test: (state, action) => {
                return action.payload.a ? undefined : ['yellow'];
            }
        }, defaultState);

        expect(reducer(['green'], { type: 'test', payload: { a: 1 } })).to.deep.equal(undefined);
    });

So I don't think there is anything to fix here since reducers should not be returning undefined in the first place, but it'll be good if an error is thrown when a reducer returns undefined so that the user knows there is a problem, otherwise the reducer might return incorrect state which might lead to unexpected results in the application

[mslipper]

@amit1911 thanks for doing the digging here. Are you still interested in implementing a PR that does error checking for cases like this, or shall I try my hand at it?