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

fix(grouping): groupByActionTypes seems to be broken - I found a fix #291

Open
1 of 4 tasks
lbragile opened this issue Feb 6, 2022 · 1 comment
Open
1 of 4 tasks

Comments

@lbragile
Copy link

lbragile commented Feb 6, 2022

I'm submitting a ...

  • Bug report
  • Feature request
  • Docs update
  • Support request => Please do not submit support requests here, see note at the top of this template.

What is the current behavior/state of the project?

From the documentation:

If you want to implement custom grouping behaviour, pass in your own function with the signature (action, currentState, previousHistory). If the return value is not null, then the new state will be grouped by that return value. If the next state is grouped into the same group as the previous state, then the two states will be grouped together in one step.

Meaning, that the action.type return for a "group" must be identical for the state to update only ONCE, returning different action.type means that the state updates each time, even if the actions are in the same "group". However, I see the following:

export function groupByActionTypes (rawActions) {
  const actions = parseActions(rawActions)
  return (action) => actions.indexOf(action.type) >= 0 ? action.type : null
}

Which returns a different value for the "group" as each action will most likely have a different type.
This will always break the following condition:

If the next state is grouped into the same group as the previous state, then the two states will be grouped together in one step

What is the desired behavior?

I found that this is fixed with the following (types are added for context only):

export function groupByActionTypes (rawActions: string | string[]) {
  const actions: string[] = parseActions(rawActions)
  return (action: AnyAction) => actions.include(action.type) ? actions[0] : null
}

In the above, if the current action is in the "group", the first action type from the group is returned (always) - rather than the current action type. In theory any actions index would work as long as it is always the same.

Thus, when a group of actions is reached, each consecutive dispatch of these actions would return the same action.type and
the condition for grouping states is met.

If this is a feature request, what is the use case for changing the behavior?

N/A

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via a gist or similar.

To reproduce, use the groupByActionTypes with debug: true and check the present state when dispatching grouped actions.
The past state should only update once if the actions are consecutive and from the same group - however it updates many times (number of actions times).

Please tell us about your environment:

  • Library version: 1.0.1
  • Redux version: 4.1.2
  • Browser: Chrome
  • Language: Typescript 4.5.5

Other information

N/A

I checked with my app and the above seems to fix things for me.
I can create a PR if needed.
Thank you for your awesome work!

@alxpez
Copy link

alxpez commented Mar 11, 2022

I've encountered this same unexpected behaviour and was wondering the feature is just not very clear in terms of phrasing, or rather if this is the expected behaviour from the devs POV (notice that in the docs examples they only use a single action type).

However, I hope it's indeed a bug! (otherwise the feature is pretty pointless 😅 )

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