Question regarding the devtools middleware source code

[devanshj]

(For #662) I don't fully understand the code nor I fully understand how the redux devtools work, my understanding is mostly based on this and this, so I have some questions.

1. If I understand correctly the message of type { type: "ACTION", payload } would have the action as the payload and not the state but yet we're setting it as the state via setState(JSON.parse(message.payload)):
   

   zustand/src/middleware/devtools.ts

   Line 116 in 833f57e

   api.setState(JSON.parse(message.payload))

   @dai-shi speculated that we might be sending the state as action object. But that's not the case, you send the action to devtools via send method that is of signature (action, state) => void, in all 3 references of devtools.send...
   

   zustand/src/middleware/devtools.ts

   Line 86 in 833f57e

   api.devtools.send(api.devtools.prefix + (name || 'action'), get())

   
   

   zustand/src/middleware/devtools.ts

   Line 106 in 833f57e

   api.devtools.send(api.devtools.prefix + 'setState', api.getState())

   
   

   zustand/src/middleware/redux.ts

   Line 32 in 833f57e

   api.devtools.send(api.devtools.prefix + action.type, get())

   ...we're always send a string as an action and not state. So that'd make the following...

   zustand/src/middleware/devtools.ts

   Line 116 in 833f57e

   api.setState(JSON.parse(message.payload))

   ...a bug, right? Am I missing something?

2. Why are we deleting api.devtools when we haven't even setted it yet? No other middleware sets it, and nor any other middleware should be allowed to set it afaiu.
   

   zustand/src/middleware/devtools.ts

   Line 80 in 833f57e

   delete api.devtools

3. Similarly why are we making this !api.devtools check here? Wouldn't api.devtools always be undefined here?

   zustand/src/middleware/devtools.ts

   Line 91 in 833f57e

   if (!api.devtools) {

4. Also devtools should overwrite setState to be namedSetState (ie the third argument acts like a name/action for the state update) which we do here...

   zustand/src/middleware/devtools.ts

   Line 89 in 833f57e

   api.setState = namedSet

   But the we overwrite namedSetState if !api.devtools (which always will be true afaict) here... and now we don't care about the third parameter, why so?

   zustand/src/middleware/devtools.ts

   Lines 93 to 109 in 833f57e

   	api.setState = <
   	K1 extends keyof S = keyof S,
   	K2 extends keyof S = K1,
   	K3 extends keyof S = K2,
   	K4 extends keyof S = K3
   	>(
   	state: PartialState<S, K1, K2, K3, K4>,
   	replace?: boolean
   	) => {
   	const newState = api.getState()
   	if (state !== newState) {
   	savedSetState(state, replace)
   	if (state !== (newState as any)[DEVTOOLS] && api.devtools) {
   	api.devtools.send(api.devtools.prefix + 'setState', api.getState())
   	}
   	}
   	}

   Okay ffs I realised we are calling the savedSetState (which will be namedSetState?) but then we don't pass the third parameter...?
   

   zustand/src/middleware/devtools.ts

   Line 104 in 833f57e

   savedSetState(state, replace)

Let me ask these questions for now. Either the code is weird or I'm being dumb and missing a major piece.

I can leave the devtools (and redux) middleware as it is but it's important to understand the runtime behavior to type things, without that I can't do much. Not to mention it'll be odd if all other pieces of codebase get polished and this remains less accessible.

[dai-shi]

I'm not super familiar with the behavior/usage of devtools. @Aslemammad can you help us?
If anybody else can help with it, please chime in.

[dai-shi]

we might be sending the state as action object. But that's not the case, you send the action to devtools via send method that is of signature (action, state) => void, in all 3 references of devtools.send...

What we send to devtools ext and what we receive from it is not identical. We don't have such actions unless with redux middleware. The code we receive action and restore the state is added recently. #608

[dai-shi]

maybe we should change/improve the behavior to accept the action which is identical to what we send. cc @Aslemammad

[devanshj]

Okay so let me give a little background, the main interactions with the devtools are:

1. Sending the action and the (computed) current state via send(action, state)
2. Listening to messages that tell us to update the state via message { type: "DISPATCH", state }
3. Listening to messages that tell us to dispatch an action via message { type: "ACTION", payload }

For 1 given we don't have action in zustand, we only do send('setState', state) and additionally we allowed the user to "name the state" via set(update, false, name) and then we'd send send(name, state)

For 2 we'd simply set(message.state)

For 3 we used to do nothing because we don't have a way to dispatch an action because we don't have actions, our actions are just "names of the update" because that is what we send.

(note that this whole thing works a little differently when we also have a redux middleware but that's not relevant for now)

Now the 3 part changed via #608 which added this code...

if (message.type === 'ACTION' && message.payload) {
  try {
    api.setState(JSON.parse(message.payload))
  } catch (e) {
    console.error(
      'please dispatch a serializable value that JSON.parse() support\n',
       e
    )
}

And why did it add it? As the PR says it was inspired by pmndrs/valtio#261 which added a similar piece of code...

if (message.type === 'ACTION' && message.payload) {
  try {
    Object.assign(proxyObject, JSON.parse(message.payload))
  } catch (e) {
    console.error(
      'please dispatch a serializable value that JSON.parse() and proxy() support\n',
       e
    )
  }
}

But here is the difference between zustand and valtio, in zustand we send names as actions but in valtio we send state change as the action:

const unsub1 = subscribe(proxyObject, (ops) => {
  const action = ops
    .filter(([_, path]) => path[0] !== DEVTOOLS)
    .map(([op, path]) => `${op}:${path.map(String).join('.')}`)
    .join(', ')

  if (!action) {
    return
  }

  if (isTimeTraveling) {
    isTimeTraveling = false
  } else {
    const snapWithoutDevtools = Object.assign({}, snapshot(proxyObject))
    delete (snapWithoutDevtools as any)[DEVTOOLS]
    devtools.send(
      {
        type: action,
        updatedAt: new Date().toLocaleString(),
      },
      snapWithoutDevtools
    )
  }
})

So pmndrs/valtio#261 was correct because valtio sends state as action but #608 was incorrect because zustand sends "names" as action.

What we send to devtools ext and what we receive from it is not identical.

Yes but in some cases it's identical and in the cases it's not JSON.parse would throw and we'd catch and let the user know that this is not supported.

---

I 90% know how the devtools work and how are we supposed to interact with it and what the behavior should be, hence in #662 I ignored the current behavior and code and did a "rewrite" instead of a "refactor". Now we have two options:

1. First understand what the current code and behavior is, find out bugs like these, then do a "refactor"
2. Let me rewrite the code (which I already did) and then I can thoroughly document in the PR how does the devtools work and what is our strategy and what we're doing. I can also add tests for the devtools which currently don't exist. And then if you want you can have someone who knows devtools review it.

I'd suggest we go with 2 because the current code, don't mind me, is a real mess haha. But the final decision is up to you ofc you're the boss.

Also If I'm rewriting and you want me to it in a separate PR, I'd preferably want to base it on #662 because I'm very particular with types. For example when we're updating state from devtools, we do it "silently" and I have typed things in a way that if we accidently do setState(message.state) instead of silentlySetState(message.state) it won't compile, so that we can catch mistakes like this compile-time.

(I'll also document a bit of "the correct way" to types in #662 at the end)

[dai-shi]

For 2 we'd simply set(message.state)

Is this currently implemented? Or, you would do so?

but in valtio we send state change as the action:

It's state itself, not state change. But, I guess you understand it correctly.

---

Overall, your suggestion sounds valid and better. My only request is to do this improvement&fix before v4, meaning before #662. (even if the typing is not precise.)

I can also add tests for the devtools which currently don't exist.

That would be really nice.

[devanshj]

Is this currently implemented? Or, you would do so?

It is currently implemented. You can see it here, first we assign message.state to newState[DEVTOOLS]...

zustand/src/middleware/devtools.ts

Line 128 in 833f57e

;(newState as any)[DEVTOOLS] = JSON.parse(message.state)

...then we api.setState it...

zustand/src/middleware/devtools.ts

Line 133 in 833f57e

api.setState((newState as any)[DEVTOOLS])

It's state itself, not state change.

No I meant the action is the state change as ops are operations (changes) afaiu and we're constructing action from them like so:

const action = ops
  .filter(([_, path]) => path[0] !== DEVTOOLS)
  .map(([op, path]) => `${op}:${path.map(String).join('.')}`)
  .join(', ')

Overall, your suggestion sounds valid and better.

Thanks for considering it!

My only request is to do this improvement&fix before v4, meaning before #662. (even if the typing is not precise.)

Okay I'll give it a try.