Mercateo / component-check

A quick introduction to exploring how components can be created in several frameworks.
Apache License 2.0
465 stars 27 forks source link

Probably shouldn't increment the counter value in state direclty #5

Closed niksosf closed 8 years ago

niksosf commented 8 years ago

Hi there, perhaps I am wrong about this, but in the Redux reducer for INCREMENT and DECREMENT, the value for the new counter for the new state is done by ++state[index] and --state[index]], respectively, I think this will modify the old state's counter's value. If we stick to the immutable approach to things with Redux, would you agree that it should be state[index]+1 and state[index] - 1 instead?

Best Regards, Nik

donaldpipowitch commented 8 years ago

Thanks for pointing this out. Yeah, I think you are right. I'll fix that soon.

lwblackledge commented 8 years ago

Could you also do this with slice and splice as opposed to the ES2015 syntax? (Mobile, sorry for the formatting and terrible names).

var newState = state.slice(); newState.splice(i,1, newState[i] + 1);

donaldpipowitch commented 8 years ago

Hi. I'm not sure what you mean. I think I do it in the same way as it is recommend by Redux?

lwblackledge commented 8 years ago

Ah yes, after reading the redux docs they do use the ES2015 rest syntax. Personally I just find it strange to modify an item at a known index by creating a new array with every item before the index, then modifying the item at the index, then adding every remaining item. I thought slice and splice were a good combination but clearly out of vogue! Please ignore this suggestion.

Thank you for the very interesting comparison as well. On Thu, Dec 24, 2015 at 2:26 AM Donald Pipowitch notifications@github.com wrote:

Hi. I'm not sure what you mean. I think I do it in the same way as it is recommend by Redux?

— Reply to this email directly or view it on GitHub https://github.com/Mercateo/component-check/issues/5#issuecomment-167060387 .

donaldpipowitch commented 8 years ago

It looks strange on the first glance, yes. I think the spread syntax is actually quite straightforward, but not the way how the array is split up. I think we need something like this:

return [
        ...state.till(index - 1),
        process(state[index]),
        ...state.from(index + 1)
      ];

Instead of

return [
        ...state.slice(0, index),
        process(state[index]),
        ...state.slice(index + 1)
      ];

It is like adding contains to String.prototype, so we don't need to abuse indexOf.