zalmoxisus / redux-devtools-extension

Redux DevTools extension.
MIT License
13.5k stars 1k forks source link

Extract LiftedState directly from the extension #618

Open ambientlight opened 5 years ago

ambientlight commented 5 years ago

I am writing an integration with reductive via communicating directly through connect API. I am able to implement most of the monitor actions, except the actions that rely on liftedState:

for now liftedState is only passed in TOOGLE_ACTION monitor action when skipping an action.

It would be great if either there is some way to explicitly retrieve liftedState from the extension API, or maybe passing it in above-mentioned action's payload. Or maybe there is some hack for now?

I can have the corresponding liftedState on my side and update it accordingly on app and monitor actions but it feels like it should be directly available from the extension.

zalmoxisus commented 5 years ago

Adding liftedState to every monitor action would have performance cost as it has to be serialized everytime. Adding a way to request it from the extension if needed or specifying for what action to include it would be reasonable.

For TOOGLE_ACTION it sends current liftedState and should get modified one (as actions starting from that toggled one will be recomputed and states will be different after that). Same is true for REORDER_ACTIONS, and we should indeed add liftedState for that too. It just doesn't work in any monitors anyway. It was implemented in https://github.com/alexkuz/redux-devtools-inspector/pull/64, but later reverted (as there was not much interest for adding that complexity in the UI). So at the moment it cannot be dispatched, and can ignore it.

SWEEP is handled on the monitor side here for non-redux, and there should be no need to do anything from client side. If that doesn't work out of the box, then you app was interpreted as redux here because you were sending liftedState instead of only store state from start. We should handle it better, expecting that non-redux implementations can do that too.

We should probably handle LOCK_CHANGES as well from monitor side. Same as for jumping, we're just assuming that the client part will handle it in the right way.

For redux we have liftedState doubled on the client side in instrumentation). The reason we're keeping a copy is to implement hot-reloading even if the extension is not in use. When the extension is not in use (its window wasn't opened) for redux the extension doesn't store liftedState history, so we don't have perf waste on serialization if not needed. There's a way to use it without that only for logging.

However for non-redux apps the recommendation is not to maintain a copy of liftedState as it would add extra complexity, especially if the state can be mutated. It should be directly available from the extension as you suggested.

ambientlight commented 5 years ago

@zalmoxisus: thank you for a detailed explanation.

Interesting, I actually don't send the liftedState from the start. I only send liftedState back on each TOOGLE_ACTION and on IMPORT_STATE. I don't think my app should be recognized as redux. I call init with plain state and then call send for each action dispatched also with plain states, they look just like:

screen shot 2018-12-24 at 12 05 24 pm

So my questions are then following:

  1. On each SWEEP dispatched from the monitor, I really do not need to dispatch anything back to extension? (since nothing happens) For now I cache the liftedState on each TOOGLE_ACTION dispatched, clean it up and send back to extension (but it has an obvious drawback that any actions dispatch after any states toggled is lost since not reflected in cached liftedState). sweep action is following: screen shot 2018-12-24 at 12 12 32 pm
  2. For LOCK_CHANGES, do I also need to dispatch anything back to the extension to have the lock state toggled. So far I could only achieve the button toggled if I explicitly send the liftedState with isLocked set on it.

The options passed to extensions are the following:

let options = ReduxDevTools.enhancerOptions(
  ~name="ReductiveTests",
  ~serialize=ReduxDevTools.serializeOptions(
    ~symbol=true,
    /* ~replacer=Helpers.serializeWithRecordKeys, */
    ()
  ),
  ~features=ReduxDevTools.enhancerFeatures(
    ~pause=true,
    ~lock=true,
    ~persist=true,
    ~export=Obj.magic("custom"),
    ~import=Obj.magic("custom"),
    ~jump=true,
    ~skip=true,
    ~reorder=false,
    ~dispatch=true,
    ~test=true,
    ()
  ),
  ()
);
zalmoxisus commented 5 years ago

@ambientlight I checked now, I see SWEEP is not working. I'll fix it, it should work from the monitor part. There's nothing necessary to do from client part, as it's just visually removing the skipped actions. I will also make the lock button to toggle from the monitor part (together with refactoring for new UI), so you don't need to send anything back, just to lock any changes from your part if possible.

ambientlight commented 5 years ago

@zalmoxisus: that would be great, looking forward, thanks!

Is it also possible to add liftedState inside REORDER_ACTIONS action?

zalmoxisus commented 5 years ago

@ambientlight sure, will add in case it will be supported in future on monitors side.

ambientlight commented 4 years ago

@zalmoxisus: how's everything been going, just checking if this issue is still relevant here?