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

Lifted actions should have IDs #134

Closed
gaearon opened this issue Sep 27, 2015 · 5 comments
Closed

Lifted actions should have IDs #134

gaearon opened this issue Sep 27, 2015 · 5 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2015

Currently, lifted PERFORM_ACTION actions don't have any IDs. This causes the lifted state to be awkwardly denormalized: skippedActions stores indexes from stagedActions and has to be adjust whenever stagedActions is reordered, timestamps suffers from the same problem. Moreover it's hard to annotate actions like requested in #60 because indexes just aren't stable.

If all lifted actions with PERFORM_ACTION type included an ID generated in performAction() lifted action creator, #60 would be solvable by the monitor storing something like collapsedByActionId: { 42: true, 100: false, etc }.

I propose the following breaking change to the lifted state shape as part of #132:

Before

{
  stagedActions: [...actions],
  timestamps: [...timestamps],
  skippedActions: { index -> bool },

  committedState: state,
  computedStates: [...{ state, error }],
  currentStateIndex: int,
  monitorState: monitorState
}

After

{
  actionsById: { id -> liftedAction }, // only for PERFORM_ACTION actions
  stagedActions: [...actionIds],
  skippedActions: { actionId -> bool },
  // note there's no need for timestamps anymore—look into actionsById[id].timestamp

  committedState: state,
  computedStates: [...{ state, error }],
  currentStateIndex: int,
  monitorState: monitorState
}

I don't think there's any point to normalizing states because that's just derived data from DevTools point of view. But for actions, it feels important to enable use cases like #60 and make code inside devTools() simpler.

@ellbee @ioss @calesce What do you think? Am I making sense?

@ioss
Copy link

ioss commented Sep 28, 2015

Makes totally sense to me!

Just one question: If I understand correctly collapsedByActionId would be stored on the Monitor (state) right?
How would monitors be able to share "action meta state"?

@ellbee
Copy link
Collaborator

ellbee commented Sep 28, 2015

👍 Makes sense to me, nice to enable these use cases without increasing the complexity of the devtools API.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 28, 2015

How would monitors be able to share "action meta state"?

Each monitor would be responsible for maintaining the state it's interested in. With #132, monitors can export reducers which will receive all the same actions devTools() receives. This lets them implement custom logic on top of it.

@ioss
Copy link

ioss commented Sep 28, 2015

Nice!

@gaearon
Copy link
Contributor Author

gaearon commented Oct 17, 2015

Fixed in vNext branch.

@gaearon gaearon closed this as completed Oct 17, 2015
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

3 participants