okwolf / hyperapp-logger

Log Hyperapp state updates and action information to the console.
MIT License
47 stars 5 forks source link

Log whole slices in next state (another approach) #23

Closed Beaglefoot closed 2 years ago

Beaglefoot commented 6 years ago

This resolves #19 saving currently implemented interface.

One existing test was modified to correspond with new output.

codecov[bot] commented 6 years ago

Codecov Report

Merging #23 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #23   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          27     40   +13     
  Branches        4      5    +1     
=====================================
+ Hits           27     40   +13
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) :arrow_up:

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 634e52b...bf190a2. Read the comment docs.

okwolf commented 6 years ago

What's the purpose of adding those extra actions with underscore prefixes? I thought all we needed was to merge the current state with the action result if it would result in a state update.

Please see #24 for my approach to this problem.

Beaglefoot commented 6 years ago

Well, these approaches do different things:

Extra actions allow to run a sequence of actions. This is required to log slices right after each action.

const state = {
  a: {
    b: {
      c: 'hi'
    }
  }
};

const actions = {
  a: {
    b: {
      someAction: () => ({ c: 'bye' }),
      log: () = (state, actions) => console.log(state),
      runSequence: () = (state, actions) => {
        actions.someAction();
        // state refers to state.a.b slice and will be printed
        // right after state change as a result of someAction()
        actions.log(state);
      }
    }
  }
};

I personally believe that #24 approach is somewhat misleading, but cannot think of a scenario where it could fail right now. It's easier to understand though.

okwolf commented 6 years ago

@Beaglefoot the issue you point out is one that is not unique to the logger. In general, you shouldn't refer to the old/stale state in an action after you have called another action. Instead, you should call another action that will receive the updated state.

What about #24 is misleading?

Beaglefoot commented 6 years ago

@okwolf By 'misleading' I mean that it actually logs out an expectation of next state not the state itself.

okwolf commented 6 years ago

@Beaglefoot By 'misleading' I mean that it actually logs out an expectation of next state not the state itself.

When actions are wrapped by an HOA they are wired in to be called in between the user-provided actions and Hyperapp state updates. Because of when this happens the state is not updated yet, but we know exactly what data is going to be merged in that was returned by the action. Here is the Hyperapp logic that drives all of this: https://github.com/hyperapp/hyperapp/blob/62feb73302da9c02d04c16670804b472609c566f/src/index.js#L120-L128

okwolf commented 2 years ago

bigger better things coming soon