fix(types)(middleware/devtools): fix action type in devtools's setState #1183

lucasrabiec · 2022-08-10T15:53:05Z

adjusted setState's actionType typing to contain payload (devtools middleware)
added appropriate info to documentation
added appropriate test (+ little tests refactor)

codesandbox-ci · 2022-08-10T15:53:35Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ccb85bf:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration
Next.js	Configuration

src/middleware/devtools.ts

tests/devtools.test.tsx

devanshj · 2022-08-10T20:09:13Z

readme.md

+    set(
+      (prev) => ({ fishes: prev.fishes + count }),
+      false,
+      { type: 'bear/addFishes', payload: count }


I think it's good to have the payload key something other than "payload" to imply "payload" is not a special key, it can be anything...

Suggested change

{ type: 'bear/addFishes', payload: count }

{ type: 'bear/addFishes', count }

It's Redux-style logging and the payload is a special key for action in Redux Devtools. IMO it would be great to keep it like that while using Redux Devtools.

Here is a link to the conversation on the pmndrs DC: https://discord.com/channels/740090768164651008/740093228904218657/1006824679111135343

In what way is it special? Like how does redux devtools treat payload key different from count key?

The main goal of this PR is logging the action's payload as the payload field, not any field name the user wants. To be consistent with the Redux convention (https://github.com/redux-utilities/flux-standard-action).
Ok, I am getting now that you want to abstract from it, right? The question is whether we want to have it consistent or to have whatever field's name the user wants but I think we should leave this decision for Zustand's owner.

Okay so as I guessed redux devtools doesn't treat payload key in any special way.

Users allowed to choose the convention they want to follow, the one you've linked is just one of the many styles out there, it's not required. The only requirement and constraint of a redux action is that it must have a type property.

I understand that you want to impose an convention for your logger, but the code being edited here affects everyone, and we can't impose a non-required convention for everyone.

I think we should leave this decision for Zustand's owner

Sure, just giving my two cents.

I don't want to impose some of my conventions. I assumed that is a well-known standard and it would be nice to be consistent with it, but not. And it's okay.

I will add [key: string]: unknown; but you mention:

in general it's good to stay away from index signatures

Could you elaborate? Is there the other way to achieve that?

Ah I missed this comment, it's mostly resolved now but I'll reply for sake of completeness...

I don't want to impose some of my conventions. I assumed that is a well-known standard and it would be nice to be consistent with it, but not. And it's okay.

I never said it was "your" convention, I'm well aware about FSA, but not everyone uses it. My point was simply this: Redux does not require users to follow FSA so we should also not.

Could you elaborate? Is there the other way to achieve that?

Now I did here #1183 (comment)

tests/devtools.test.tsx

dai-shi · 2022-08-11T23:15:20Z

@lucasrabiec Thanks for opening a PR.
@devanshj Thanks for the review!

#1183 (comment)

We shouldn't follow FSA. payload is not special. It's up to users.
{ type: 'add', by: number } should be accepted as an action.

lucasrabiec · 2022-08-12T06:17:13Z

Okay, thanks for the clarification, I will add amendments.

dai-shi

LGTM

devanshj · 2022-08-12T13:49:24Z

Fwiw my suggestion here #1183 (comment) is ignored.

It's important to understand extends { type: unknown } and { type: unknown, [_: string]: unknown } are not the same type.

We don't require an index signature on the action, and requiring it will create problems like it does in the following...

declare const dispatch1:
  <A extends { type: unknown }>(a: A) => void

declare const dispatch2:
  (a: { type: unknown, [_: string]: unknown }) => void

declare const fooAction1: FooAction1
interface FooAction1 {
  type: "FOO",
  foo: number
}

declare const fooAction2: FooAction2
type FooAction2 = {
  type: "FOO"
  foo: number
}

dispatch1(fooAction1)
dispatch1(fooAction2)
dispatch2(fooAction1)
//        ~~~~~~~~~~[1]
dispatch2(fooAction2)

// [1]:
// Argument of type 'FooAction1' is not assignable to parameter of type '{ [_: string]: unknown; type: unknown; }'.
//   Index signature for type 'string' is missing in type 'FooAction1'.

As you can see fooAction1 doesn't compile with dispatch2 implying dispatch1 is the correct way to type dispatch. And of course I've just taken dispatch as a minimal example, same applies in our case too.

So we should use extends { type: unknown } as I suggested in my linked comment.

And in case linter complains that A is "unused" then the linter is wrong because it's clearly "used" and has a purpose in the way I showed, so it's correct to suppress the warning. I won't recommend renaming A to _A if that silents the linter, A is public here having an underscore will imply otherwise.

dai-shi

#1183 (comment)

Oh, I didn't follow that thread. Yeah, I wondered if extends ... should work. Let's do that.

lucasrabiec · 2022-08-12T15:44:19Z

@devanshj I didn't know about this difference, thank you for an explanation. I added a change, please take a look if it fits with your comment.

devanshj · 2022-08-12T19:31:36Z

src/middleware/devtools.ts

@@ -43,8 +43,8 @@ type StoreDevtools<S> = S extends {
  setState: (...a: infer A) => infer Sr
 }
  ? {
-      setState(
-        ...a: [...a: TakeTwo<A>, actionType?: string | { type: unknown }]
+      setState<AT extends string | { type: unknown; [key: string]: unknown }>(


This is still incorrect, we don't need or want an index signature at all. Maybe my suggestion code snippet here #1183 (comment) was confusing because of duplicate As?

And also let's rename original A to Sa instead of using AT so that the public generic is seen as A.

type StoreDevtools<S> = S extends { - setState: (...a: infer A) => infer Sr + setState: (...a: infer Sa) => infer Sr } ? { - setState( + setState<A extends string | { type: unknown }>( - ...a: [...a: TakeTwo<A>, actionType?: string | { type: unknown }] + ...a: [...a: TakeTwo<Sa>, action?: A] ): Sr } : never

Maybe I missed something but when we remove index signature we still won't be able to add action's payload: Argument of type '{ type: string; count: number; }' is not assignable to parameter of type 'string | { type: unknown; } | undefined'.

That's not true, I tried making the change locally on your branch and it compiles. You're probably not making the exact change I showed above. Make the exact change and push, we'll see if the CI complains.

Edit: Here's the repo with the change https://gitpod.io#snapshot/f6d9f74d-05cd-446b-8137-b96c7b47c6bc

Ok, my bad. I didn't mention that the error occurs if devtools are used with immer:

const createBearSlice: StateCreator< { bears: { count: number } }, [['zustand/devtools', never], ['zustand/immer', never]], [], { count: number } > = (set) => ({ count: 0, setBears: (count: number) => set( (state) => { state.bears.count = count }, false, { type: 'bears/setBears', count } // <-- TS2345 ), })

I added some changes also to immer.ts. Could you take a look at them?

It's recommended to use immer middleware before devtools, so that would fix the problem. You can change the test to use immer before devtools. I'll also open a PR to add this recommendation in the typescript doc.

As for the change you made in immer types, it's not correct, nor is it needed, so please revert it.

If we want to support using devtools before immer than the solution is much more complex. I'll see if I can open a PR for it later, but for now we don't need to support using devtools before immer

Also please don't make changes that you don't understand, it's okay if you don't fully understand the types here, it's better to ask for a solution than to guess a solution.

Mistakes can be corrected like I did here #1183 (comment). But the change you made is entirely incorrect, there's nothing to correct there. So first you will have to tell why you think it's correct, and then I can tell what is wrong in your understanding. But unfortunately, I'm not available for such OSS interactions (hence I didn't ask you why you think it's correct), especially when I can see the person did a guesswork. For me guesswork is like "I come up with a change from my incomplete understanding, it works for the given case, so it must be the right change". And it's okay to do guesswork in some situations, but it's not ideal. And it's also okay if you don't think it was guesswork, we can agree to disagree.

For now I can give you just one false positive case... With your change, immer's setState will accept an action even when devtools is not present... The following code should not compile, but with your change it'll.

import create from 'zustand' import { immer } from 'zustand/middleware' interface BearState { bears: number increase: (by: number) => void } const useBearStore = create<BearState>()(immer((set) => ({ bears: 0, increase: (by) => set((state) => ({ bears: state.bears + by }), false, "increase"), }) ))

Maybe I should have pointed this out before itself, my apologies, but there were many more things incorrect, so I skipped it entirely.

If you want to understand the types then see #710. Using immer before devtools is recommended precisely so that we don't have to do anything extra in immer types to support cases like this.

No offense taken or meant.

Ok, I expressed myself wrong, it would be good to give an example when such a solution fails, which you did, so thank you. Now I see why it's a bad solution. Thanks for pointing out #710 I will dig into it. To be honest, these typings which you did is not the easiest to understand without fully digging into them (which can be time-consuming), but it's definitely a commendable effort of work (+1). It would be good to add that issue and PR in the documentation for development.

it would be good to give an example when such a solution fails

Yes I kinda agree, hence I made the edit...

Maybe I should have pointed this out before itself, my apologies

To be honest, these typings which you did is not the easiest to understand without fully digging into them (which can be time-consuming)

Yes, and hence users or contributors are not required to understand them. Only if one wishes to make a significant types contribution then it's required to understand them.

it's definitely a commendable effort of work (+1)

Thanks!

It would be good to add that issue and PR in the documentation for development.

It's actually already there in the typescript doc in the "advanced usage" section.

It's actually already there in the typescript doc in the "advanced usage" section.

Woah, my bad, didn't noticed that 😞

No problems!

devanshj · 2022-08-12T20:19:10Z

And maybe it's better to title the PR more appropriately, some suggestions...

"fix(types): allow passing an action with additional properties other than type to devtools's setState"
"fix(types): circumvent excess-property-check for action in devtools's setState"
"fix(types): fix action type in devtools's setState"
etc

lucasrabiec · 2022-08-12T21:22:29Z

And maybe it's better to title the PR more appropriately, some suggestions...

"fix(types): allow passing an action with additional properties other than type to devtools's setState"

"fix(types): circumvent excess-property-check for action in devtools's setState"

"fix(types): fix action type in devtools's setState"

etc

Good point, TY.

This reverts commit 50ce327.

lucasrabiec · 2022-08-14T19:11:57Z

@dai-shi @devanshj I think it is ready for merge, thanks for discussion and constructive critisism.

devanshj · 2022-08-14T19:29:47Z

tests/middlewareTypes.test.tsx

+          (set, get) => ({
+            count: 0,
+            inc: () =>
+              set((state) => {


Let's add an action with payload to this set call (like you did previously) as a test for types.

devanshj · 2022-08-15T16:19:03Z

tests/middlewareTypes.test.tsx

+    TestComponent
+  })
+
+  it('immer & devtools, action with type and payload', () => {


My suggestion was to add action in the above set call itself xD, another test case wasn't necessary, but I think it doesn't hurt much to have one, so it's okay I guess.

Haha, okay xD I will change it.

dai-shi

Thanks for working on this!

tests/middlewareTypes.test.tsx

added payload to setState() in devtools middleware

ea70466

devanshj reviewed Aug 10, 2022

View reviewed changes

src/middleware/devtools.ts Outdated Show resolved Hide resolved

devanshj reviewed Aug 10, 2022

View reviewed changes

tests/devtools.test.tsx Outdated Show resolved Hide resolved

devanshj reviewed Aug 10, 2022

View reviewed changes

adjusted tests

9bbf125

devanshj reviewed Aug 11, 2022

View reviewed changes

tests/devtools.test.tsx Outdated Show resolved Hide resolved

lucasrabiec added 2 commits August 12, 2022 09:22

changed payload to any property name

bcc5e0b

adjusted docs

8e79ade

dai-shi approved these changes Aug 12, 2022

View reviewed changes

prettier readme.md

3bb1fc8

dai-shi requested changes Aug 12, 2022

View reviewed changes

adjusted for extends

ce2a715

devanshj suggested changes Aug 12, 2022

View reviewed changes

lucasrabiec changed the title ~~Added payload to setState() in devtools middleware~~ fix(types): fix action type in devtools's setState Aug 12, 2022

adjusted immer's setState

50ce327

lucasrabiec changed the title ~~fix(types): fix action type in devtools's setState~~ fix(types): fix action type in devtools's and immer's setState Aug 14, 2022

lucasrabiec added 2 commits August 14, 2022 17:54

Revert "adjusted immer's setState"

a6b8c35

This reverts commit 50ce327.

changed order of immer and devtools in tests

f8fe591

lucasrabiec changed the title ~~fix(types): fix action type in devtools's and immer's setState~~ fix(types): fix action type in devtools's setState Aug 14, 2022

devanshj suggested changes Aug 14, 2022

View reviewed changes

added test for action with payload type in immer/devtools

f2f4ef8

devanshj reviewed Aug 15, 2022

View reviewed changes

devanshj approved these changes Aug 15, 2022

View reviewed changes

removed unnecessary test

ebbee73

dai-shi approved these changes Aug 17, 2022

View reviewed changes

tests/middlewareTypes.test.tsx Outdated Show resolved Hide resolved

Update tests/middlewareTypes.test.tsx

ccb85bf

dai-shi added this to the v4.1.0 milestone Aug 18, 2022

dai-shi changed the title ~~fix(types): fix action type in devtools's setState~~ fix(types)(middleware/devtools): fix action type in devtools's setState Aug 18, 2022

dai-shi merged commit ed12c7e into pmndrs:main Aug 18, 2022

lucasrabiec deleted the devtools-action-payload branch August 20, 2022 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(types)(middleware/devtools): fix action type in devtools's setState #1183

fix(types)(middleware/devtools): fix action type in devtools's setState #1183

lucasrabiec commented Aug 10, 2022

codesandbox-ci bot commented Aug 10, 2022 •

edited

Loading

devanshj Aug 10, 2022

lucasrabiec Aug 11, 2022

devanshj Aug 11, 2022 •

edited

Loading

lucasrabiec Aug 11, 2022

devanshj Aug 11, 2022

lucasrabiec Aug 12, 2022

devanshj Aug 12, 2022 •

edited

Loading

dai-shi commented Aug 11, 2022

lucasrabiec commented Aug 12, 2022

dai-shi left a comment

devanshj commented Aug 12, 2022 •

edited

Loading

dai-shi left a comment

lucasrabiec commented Aug 12, 2022

devanshj Aug 12, 2022 •

edited

Loading

lucasrabiec Aug 12, 2022 •

edited

Loading

devanshj Aug 12, 2022 •

edited

Loading

lucasrabiec Aug 14, 2022

devanshj Aug 14, 2022 •

edited

Loading

devanshj Aug 14, 2022 •

edited

Loading

lucasrabiec Aug 14, 2022

devanshj Aug 14, 2022

lucasrabiec Aug 14, 2022

devanshj Aug 14, 2022

devanshj commented Aug 12, 2022

lucasrabiec commented Aug 12, 2022

lucasrabiec commented Aug 14, 2022

devanshj Aug 14, 2022

devanshj Aug 15, 2022

lucasrabiec Aug 15, 2022

dai-shi left a comment

	{ type: 'bear/addFishes', payload: count }
	{ type: 'bear/addFishes', count }

fix(types)(middleware/devtools): fix action type in devtools's setState #1183

fix(types)(middleware/devtools): fix action type in devtools's setState #1183

Conversation

lucasrabiec commented Aug 10, 2022

codesandbox-ci bot commented Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanshj Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanshj Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

dai-shi commented Aug 11, 2022

lucasrabiec commented Aug 12, 2022

dai-shi left a comment

Choose a reason for hiding this comment

devanshj commented Aug 12, 2022 • edited Loading

dai-shi left a comment

Choose a reason for hiding this comment

lucasrabiec commented Aug 12, 2022

devanshj Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

lucasrabiec Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

devanshj Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanshj Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

devanshj Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanshj commented Aug 12, 2022

lucasrabiec commented Aug 12, 2022

lucasrabiec commented Aug 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi left a comment

Choose a reason for hiding this comment

codesandbox-ci bot commented Aug 10, 2022 •

edited

Loading

devanshj Aug 11, 2022 •

edited

Loading

devanshj Aug 12, 2022 •

edited

Loading

devanshj commented Aug 12, 2022 •

edited

Loading

devanshj Aug 12, 2022 •

edited

Loading

lucasrabiec Aug 12, 2022 •

edited

Loading

devanshj Aug 12, 2022 •

edited

Loading

devanshj Aug 14, 2022 •

edited

Loading

devanshj Aug 14, 2022 •

edited

Loading