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

Incorrect next state with actions updating slices of state #19

Open
Beaglefoot opened this issue Feb 27, 2018 · 2 comments
Open

Incorrect next state with actions updating slices of state #19

Beaglefoot opened this issue Feb 27, 2018 · 2 comments

Comments

@Beaglefoot
Copy link

Imagine the following scenario:

const state = {
  counters: {
    count1: 1,
    count2: 2,
    count3: 3
  }
};

const actions = {
  counters: {
    inc1: () => ({ count1 }) => ({ count1: count1 + 1 })
  }
};

Let's run inc1() action and here's what we see...

prev state:

{
  count1: 1,
  count2: 2,
  count3: 3
}

next state:

{ count1: 2 }

What's expected next state:

{
  count1: 2,
  count2: 2,
  count3: 3
}

Right now logger simply returns the result of an action, and I strongly believe that it should return merged slice of a state.
Here's an illustration

BTW, thanks for logger!

@okwolf
Copy link
Owner

okwolf commented Mar 1, 2018

The root cause, of course, is that Hyperapp performs a shallow merge of your action result/return with the existing state in the same slice and finally returns only the partial state that was merged in to the caller. I can think of three ways of solving this:

  1. Update the default logger message and documentation to reflect that we are seeing the action result instead of the next state.
  2. Actually perform the same merge as Hyperapp and use the entire next state slice value. This will be duplicating a very small bit of code already in Hyperapp, but it's not likely to change much.
  3. Petition Hyperapp core to return the merged new state slice instead of just the action result from actions that update the state, and this works with no change at all. This would be a breaking change and not very likely to be accepted.

Beaglefoot added a commit to Beaglefoot/logger that referenced this issue Mar 1, 2018
@Beaglefoot
Copy link
Author

Beaglefoot commented Mar 1, 2018

There is another solution, check #21

Well, I played around with this a little bit and it seems that some actions get lost when asynchrony is involved. Please ignore...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment