Make combineActions works with nested handleActions #294

Kerumen · 2018-05-03T18:31:17Z

Fixes #241 (I used the code @Parakleta suggested)

It allows nesting of actions in handleActions after a combineActions (very useful for loadings):

const reducer = handleActions(
  {
    [combineActions(apiCall1, apiCall2)]: {
      LOADING: () => ({ loading: true }),
    }
  },
  { loading: false }
);

I had some troubles to write a clean test, let me know if you want to improve it.

codecov · 2018-05-03T18:35:08Z

Codecov Report

Merging #294 into master will increase coverage by 0.15%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
+ Coverage   95.45%   95.61%   +0.15%     
==========================================
  Files          14       14              
  Lines         110      114       +4     
  Branches       36       36              
==========================================
+ Hits          105      109       +4     
  Misses          4        4              
  Partials        1        1

Impacted Files	Coverage Δ
src/utils/flattenWhenNode.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 652668c...9859b25. Read the comment docs.

timche · 2018-05-03T18:35:14Z

Thanks! I'll look into this asap.

timche

Could you explain me from an API perspective what exactly is being added here and why that isn't possible now? I was out of this library for longer time and many things have been added since then. After reading #241 and your example I'm not 100% sure and also using the term "nested" in combination with handleActions and combineActions confuses me a little bit. Thanks.

I think it's best if you could update the documentation accordingly.

timche · 2018-05-04T10:20:31Z

test/handleActions.test.js

+    }
+  });
+
+  // NOTE: We should be using combineReducers in production, but this is just a test.


What does this comment mean, because

combineReducers doesn't exist

if combineActions is meant, it is being used

I just copy-pasted the previous test 🙃

So we can remove it as I don't see that it makes any sense.

Kerumen · 2018-05-04T13:30:22Z

@timche To be honest, I wanted to use a "nested" handleActions with combineActions. I found out it wasn't working (only the last action was handled) so I search through issues and found the #241. Someone provided a code solution and after some tests in my local project, that was perfectly working. I filled this PR.

Currently, if you write your handleActions like this:

handleActions({
  NOT_NESTED: function,
  [combineActions(action1, action2)]: {
    NESTED: function,
  }
})

Only action2/NESTED will be handled. Not action1/NESTED.

timche · 2018-05-04T13:42:20Z

Thanks @Kerumen for explaining. I'm not questioning your pull request, it's totally fine and appreciate it. Please don't get me wrong, I'm just trying to understand and to make everyone clear what we are trying to achieve as this must also be documented in the documentation and change log. Also regarding the left-over comment in the test, it doesn't mean we can't do better and fix it, right?

Do I understand correctly that combineActions is not working as expected and should be considered as a bug or is this just adding a new feature where we can additionally pass an object beside a function and it was just working with the last action accidentally?

Kerumen · 2018-05-04T14:03:46Z

I don't know how the library was designed. I'm sure that this is not documented for combineActions. But nested handleActions works and is documented:

If reducerMap has a recursive structure, its leaves are used as reducers, and the action type for each leaf is the path to that leaf.

So a combination of both should work?

timche · 2018-05-04T14:16:18Z

Alright, so it really sounds more like a bug to me. So we are basically fixing combineActions to work with handleActions as advertised.

timche · 2018-05-04T14:21:19Z

test/handleActions.test.js

+        DECREMENT: amount => ({ amount: -amount })
+      }
+    }
+  });


Please check the double usage of createAction in L536 and L542. Shouldn't it be only at L542? counter and sameCounter can be destructured there instead too.

Yes maybe I should rename the actions of L536. In my app I have an API call (which is an action) and I handle SUCCESS, LOADING.. with combineActions.
So I dispatch API_CALL and handle API_CALL_SUCCESS. This is why I have 2 createActions. I tried to reproduce this here but maybe it's confusing. 😐

Alright, but still couldn't you just remove L536 and do this instead?

const { app } = createActions({ APP: { COUNTER: { INCREMENT: amount => ({ amount }), DECREMENT: amount => ({ amount: -amount }) }, SAME_COUNTER: { INCREMENT: amount => ({ amount }), DECREMENT: amount => ({ amount: -amount }) } } }); const { counter, sameCounter } = app

@timche This doesn't work because of line L558: combineActions(counter, sameCounter). combineActions expect actions, not keys. I can rewrite L536 with:

const app: { counter, sameCounter } = createActions('APP/COUNTER', 'APP/SAME_COUNTER')

..if you prefer.
Or totally change action names and do something like API_CALL and API_CALL_SUCCESS?

I know this is not very clear but the initials dispatched (and combined) actions have to be different than the one nested in the combine.

Sorry, didn't have much time to look into that and try it out myself. I just want it to be changed so the code makes sense. Doesn't matter how and to what, it just must be readable and clear.

It's still not very clear from my perspective because of using undefined in API_CALL_1: { LOADING: undefined }. Why is it defined as undefined? It just doesn't look right and is also not documented.

It's totally documented: https://redux-actions.js.org/api-reference/createaction-s#createaction-type-payloadcreator and also used in an example below

SET: undefined // given undefined, the identity function will be used

But if you prefer I can write createAction('API_CALL_1/LOADING'). This way, I use explicitly the / separator which is implicit in the object form.

I know that undefined will use the identity function instead, but I'm more concerned about using createActions twice and the first one is just being used for creating action types and the second to create action creators which is overall looks quite confusing imo.

I know.. As I said, in a real app, the LOADING action would have been created else where (in a middleware for example) so it won't be confusing.

However, this is a simple test ensuring the fix works, maybe we can pass through and improve it later?

I've changed it a few times and I don't have many options left. If you propose me something, I would be happy to include it, I'm currently out of ideas.

Yeah, I think this is too much nitpicking from my side, sorry. After all, this issue is fixed. Thanks!

timche

Happy to merge it asap when finished discussing about the test.

Kerumen · 2018-05-13T15:23:28Z

@timche Friendly ping, I made a comment in the diff :)

Kerumen · 2018-05-14T10:15:01Z

@timche I've renamed the variables to better match my app and my intention.
Here, it's weird because the LOADING action is created next to the initial action. In a real app, the LOADING would have been created else where (in a middleware for example).

Is that clearer? Sorry for that, I can't find a proper way to write this test..

Kerumen · 2018-05-24T12:24:15Z

@timche Can you publish a new version with this change on npm please? Thanks!

timche · 2018-05-27T23:43:11Z

Sorry for the delay. Published.

make combineActions works with nested handleActions

4dbecf9

timche reviewed May 4, 2018

View reviewed changes

timche approved these changes May 4, 2018

View reviewed changes

timche requested changes May 7, 2018

View reviewed changes

Rename variables in test

9859b25

timche approved these changes May 15, 2018

View reviewed changes

timche merged commit 0d1b1f7 into redux-utilities:master May 15, 2018

snyk-bot mentioned this pull request Mar 6, 2020

[Snyk] Upgrade redux-actions from 0.10.1 to 2.6.5 kogg/InstantLogoSearch#234

Open

apiiro-snyk mentioned this pull request Jun 20, 2024

[Snyk] Upgrade redux-actions from 2.3.0 to 2.6.5 apiiro-snyk/patient-browser#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make combineActions works with nested handleActions #294

Make combineActions works with nested handleActions #294

Kerumen commented May 3, 2018 •

edited

Loading

codecov bot commented May 3, 2018 •

edited

Loading

timche commented May 3, 2018

timche left a comment •

edited

Loading

timche May 4, 2018

Kerumen May 4, 2018

timche May 4, 2018

Kerumen commented May 4, 2018 •

edited

Loading

timche commented May 4, 2018

Kerumen commented May 4, 2018

timche commented May 4, 2018

timche May 4, 2018

Kerumen May 4, 2018

timche May 7, 2018

Kerumen May 8, 2018

timche May 14, 2018 •

edited

Loading

timche May 14, 2018 •

edited

Loading

Kerumen May 14, 2018

timche May 15, 2018

Kerumen May 15, 2018 •

edited

Loading

timche May 15, 2018

timche left a comment

Kerumen commented May 13, 2018

Kerumen commented May 14, 2018 •

edited

Loading

Kerumen commented May 24, 2018

timche commented May 27, 2018

Make combineActions works with nested handleActions #294

Make combineActions works with nested handleActions #294

Conversation

Kerumen commented May 3, 2018 • edited Loading

codecov bot commented May 3, 2018 • edited Loading

Codecov Report

timche commented May 3, 2018

timche left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kerumen commented May 4, 2018 • edited Loading

timche commented May 4, 2018

Kerumen commented May 4, 2018

timche commented May 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timche May 14, 2018 • edited Loading

Choose a reason for hiding this comment

timche May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kerumen May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timche left a comment

Choose a reason for hiding this comment

Kerumen commented May 13, 2018

Kerumen commented May 14, 2018 • edited Loading

Kerumen commented May 24, 2018

timche commented May 27, 2018

Kerumen commented May 3, 2018 •

edited

Loading

codecov bot commented May 3, 2018 •

edited

Loading

timche left a comment •

edited

Loading

Kerumen commented May 4, 2018 •

edited

Loading

timche May 14, 2018 •

edited

Loading

timche May 14, 2018 •

edited

Loading

Kerumen May 15, 2018 •

edited

Loading

Kerumen commented May 14, 2018 •

edited

Loading