add prepareAction option to createAction #149

phryneas · 2019-06-22T21:33:57Z

This is a rough WIP for the feature requested in #148. Essentially, it allows to pass a prepare function with the signature (originalPayload: OriginalPayload) => {payload: Payload; meta? : Meta} to modify the Payload and potentially add a Meta to the created action. You can see examples of use in the type tests I added.

This is working, but needs tuning around the types - but before that I would like some feedback if I'm going in the right direction with this.

@markerikson @tanhauhau what do you think?

netlify · 2019-06-22T21:36:09Z

Deploy preview for redux-starter-kit-docs ready!

Built with commit 5ed1286

https://deploy-preview-149--redux-starter-kit-docs.netlify.com

markerikson · 2019-06-23T05:56:17Z

Skimming the code, I have two immediate thoughts.

First, I don't think this is quite what I had in mind. What I'm really looking for is the ability to define a function that takes whatever parameters I declare, and turns those into the payload. Typically, folks who write action creators turn multiple arguments into a single object payload, or do some calculations in the action creator like generating an ID.

A couple hypothetical examples:

const payloadFromArgs = (a : string, b : number) => ({a, b});

const payloadIdGenerator = (name : string) => {
     const id = uuid();
     return {id, name};
}

Second: ow, the types are starting to hurt my eyes :(

tanhauhau · 2019-06-23T07:04:03Z

I personally think that redux-action's createAction api is flexible and great, but man , when I look at their types from DefinitelyTyped, it's kind of pain to read.

phryneas · 2019-06-23T07:58:45Z

Skimming the code, I have two immediate thoughts.

First, I don't think this is quite what I had in mind. What I'm really looking for is the ability to define a function that takes whatever parameters I declare, and turns those into the payload. Typically, folks who write action creators turn multiple arguments into a single object payload, or do some calculations in the action creator like generating an ID.

Ah, that's very valid, I didn't think of that. So if the prepareAction method would take multiple arguments, the general direction for the API would be okay?

Second: ow, the types are starting to hurt my eyes :(

Yup :( Mine too :(

Problem is: We can't really touch any exported pre-existing type other than adding more generics at the end because people might be using them. And I might have broken that rule a few times anyways. That's why I meant this still needs a lot of clean-up.
That leads (in addition to this already being very exhaustive types anyways) to weird generic ones, or new types that do almost the same. :/

Also, the distinction between PayloadAction and SliceAction doesn't really help. Do you know why that isn't one type?

phryneas · 2019-06-23T08:03:37Z

I personally think that redux-action's createAction api is flexible and great, but man , when I look at their types from DefinitelyTyped, it's kind of pain to read.

Hm, yeah, that might also be an option. I think the types might only hurt that much because of backwards compatiblity to older TS versions. A problem would be getting a good API for createSlice that doesn't end up in a combinatoric explosion of types (only reducer, reducer with payloadCreator, reducer with metaCreator, reducer with payloadCreator and metaCreator - that might lead to a LOT of types :( ) - this is why I chose this approach in the beginning.

markerikson · 2019-06-23T08:24:03Z

I have no idea why any particular types exist in this lib atm - I didn't do the TS conversion.

As a general observation: breakages are never good, but this lib is pre-1.0 for a reason. If it's necessary to break existing types to get things to work better, go ahead.

phryneas · 2019-06-23T12:52:48Z

@markerikson @tanhauhau
So here's a second definition with the API initially described in #148. (Again, not very clean yet - I'm happy to have gotten it to work so far, this stuff is intense)

https://github.com/reduxjs/redux-starter-kit/compare/master...phryneas:prepareAction2?expand=1

The TypeScript doesn't look much more readable :/

So, assuming adding the ...args in this branch wouldn't require much effort - what do you think? Which branch is worth continuing?

@tanhauhau - do you want to give it a try so we have a third alternative to choose from?

tanhauhau · 2019-06-23T14:44:49Z

🤔 let me try.. may take a while to figure out the types

markerikson · 2019-07-06T17:10:01Z

Anyone had a chance to think through this further?

markerikson · 2019-07-06T17:14:47Z

API-design wise: would it be better to have separate creator functions for payload and meta, or somehow have one function that must return payload and could return meta?

phryneas · 2019-07-06T19:50:11Z

Anyone had a chance to think through this further?

I was still waiting if @tanhauhau comes up with something. But if you decide to go either route I'd be happy to try and simplify one of mine solutions as well.

API-design wise: would it be better to have separate creator functions for payload and meta, or somehow have one function that must return payload and could return meta?

Typing-wise, I still think it would be easier to have one function there instead of two, as there's already enough different conditions flying around. Also I guess people wouldn't care too much if they had to also return the payload even if they just wanted to change the meta.

There could also be a third alternative - what do you think of something like this? Not really sure if I like it myself but may be worth a thought.
https://codesandbox.io/s/createaction-api-experiment-fz0p1

tanhauhau · 2019-07-13T04:40:55Z

@markerikson @phryneas sorry for keep you waiting, i've come up nothing. 😢
man its hard to type a flexible api

phryneas · 2019-07-13T16:48:24Z

@markerikson @phryneas sorry for keep you waiting, i've come up nothing. cry
man its hard to type a flexible api

@tanhauhau no worries, this one is really hard to type.

@markerikson - have you come up with a decision on which API to settle? I'll happily try to simplify (or at least streamline a bit) the typings for any approach.

markerikson · 2019-07-13T21:30:38Z

Mmm... yeah, let's go with something like (...args) => ({payload, meta?}) for the prepare callback.

…slices

phryneas · 2019-07-20T16:23:27Z

Okay, as so much changed with #157 , I completely redid the PR. (old diff can be found here)

I also added a lot more tests, but there are still some cases where I'll need more tests.

I'm afraid I couldn't find a way to type it so that the case prepare result & reducer input not compatible throws a type error so that test currently fails . Might try a few other things in the next days though.

markerikson · 2019-07-20T23:51:11Z

Hmm. FWIW, the "prepare" thing shouldn't be part of createReducer() itself, as it's really a question of what the action creator accepts and returns. So, just changes to createAction and createSlice should be necessary.

phryneas · 2019-07-22T13:06:42Z

Good point.
As I just stumbled about it:
What would you think about adding something like https://github.com/joonhocho/tsdef as a dependency? That might make the types a lot more readable. (although that's no guarantee, might have to try)

markerikson · 2019-07-22T18:00:00Z

I have no problems with using some of those typedefs if they make sense. But, I'd hesitate on adding it as a specific dependency. Seems like that kind of thing would be easily copy-pastable into our own code instead.

markerikson · 2019-07-27T17:24:20Z

@phryneas : think you have time to push this ahead? I'd love to get it in relatively soon.

phryneas · 2019-07-27T17:43:21Z

@markerikson had a chaotic week but I have some spare hours this evening so I'll put some time in.

markerikson · 2019-07-27T17:45:56Z

No worries, just checking!

phryneas · 2019-07-27T20:55:52Z

@markerikson okay, by now this should be fairly complete.

Now there's just the question of that one failing test. I'd love to have that behaviour of an error being thrown there, but I don't know how to type it - if the reducer and prepare methods have differing Payload types, TS just seems to widen the type to any instead of showing an error.

The only idea to solve that would be to export another enhancedCaseReducer method that essentially looks like

function enhanceCaseReducer<S, P>(
  reducer: CaseReducer<S, PayloadAction<P>>,
  prepare: PrepareAction<P>
): EnhancedCaseReducer<S, PayloadAction<P>> {
  return { reducer, prepare };
}

and people would use it like this:

createSlice({
  slice: 'test',
  initialState: 0;
  reducers: {
    incrementMultiple: enhanceCaseReducer(
      (state, action: PayloadAction<number>) => state + action.payload,
      (a: number, b: number)  => ({payload: a+b})
    )
  }
})

that would be typesafe.

What do you think?

markerikson · 2019-07-27T21:48:12Z

I'd really rather not have any special exports just for TS usage.

Stupid TS question: is it somehow possible to throw an error if a type has turned into any or something?

phryneas · 2019-07-28T00:53:59Z

Stupid TS question: is it somehow possible to throw an error if a type has turned into any or something?

It couldn't distinguish between loosening an inferred type vs. manually typing as any. So if we did something like that, it wouldn't be possible to manually any-type enhanced reducers any more.

So I'd suggest against that.

Maybe I'm just missing something with the typings there - but I've tried about 10 different versions and I'm pretty much out of ideas. Are there other people you can ping on that? Maybe a second opinion could really help.

markerikson · 2019-07-28T01:22:40Z

Tweeted for help, and also paging the usual suspects: @Dudeonyx , @denisw , @Jessidhia , @nickmccurdy

nickserv · 2019-07-28T01:48:55Z

I read the thread and I’ll try to help, but I don’t think I’m understanding the goal here. Why not just do the ...arts form as @markerikson originally suggested and let users figure out how that maps to their action creator type? And what’s an example of code that would have a false positive in the typechecker with the current type implementation (where two different payloads match and infer to any when they shouldn’t match)?

markerikson · 2019-07-28T01:53:32Z

@nickmccurdy : see this non-working types test:

https://github.com/reduxjs/redux-starter-kit/pull/149/files#diff-d3f6ba32c4f3b495b630095111a617e9R162-R186

The prepare callback is returning a number, while the corresponding reducer is expecting a string. Instead of erroring, this is falling back to any.

nickserv · 2019-07-28T02:09:14Z

That link seems to be broken (just goes to top of PR diff), is this what you're referring to? https://github.com/reduxjs/redux-starter-kit/pull/149/files#diff-d3f6ba32c4f3b495b630095111a617e9R71-R76

markerikson · 2019-07-28T02:28:32Z

bah, that link doesn't work either :)

Direct file link:

https://github.com/reduxjs/redux-starter-kit/blob/8ec73956be2e685a2b7ce0a92544188c88a3b3c0/type-tests/files/createSlice.typetest.ts#L162-L186

nickserv · 2019-07-28T08:38:15Z

It's not widening the type, it's using the default type parameter value of any because no type parameter is given to be inferred.

The type of the reducer property is not given in this test code, so it is inferred and matched against the type CaseReducer<S, A>. With no generic parameter type given for A, it defaults to PayloadAction because of A extends PayloadAction. With no generic parameter type given to PayloadAction, it defaults to any because of P = any.

type EnhancedCaseReducer<S, A extends PayloadAction<P>, P> = {

nickserv · 2019-07-28T08:42:26Z

On an unrelated note regarding test maintenance, typings-tester makes it difficult to troubleshoot what type error is happening in the tests. Instead, I'd recommend using Microsoft's official dtslint command line tool with // $ExpectError instead of // typings:expect-error.

phryneas · 2019-07-28T09:30:45Z

It's not widening the type, it's using the default type parameter value of any because no type parameter is given to be inferred.

The type of the reducer property is not given in this test code, so it is inferred and matched against the type CaseReducer<S, A>. With no generic parameter type given for A, it defaults to PayloadAction because of A extends PayloadAction. With no generic parameter type given to PayloadAction, it defaults to any because of P = any.

type EnhancedCaseReducer<S, A extends PayloadAction<P>, P> = {

Hm, while that makes sense, it doesn't seem to be working for me - could you share a working example?

nickserv · 2019-07-28T12:36:14Z

Sorry, this isn't nearly as simple as I thought. I've been trying to pass all these types in createSlice.ts so they wouldn't default to any, and I seem to have the same issue.

…y prepare and the type accepted by reducer don't agree.

phryneas · 2019-07-28T19:14:30Z

@dragomirtitian solved this - https://twitter.com/TitianCernicova/status/1155549716777709570

Thank you so much Titian!

markerikson · 2019-07-28T19:21:41Z

Yay! Busy with some other stuff atm, but I'll have to try it out in the next day or two to see how it behaves.

phryneas · 2019-07-28T19:22:54Z

No hurry :)

markerikson · 2019-07-29T03:46:26Z

src/createAction.ts

+> = {
+  type: T
+} & (PA extends (...args: any[]) => any
+  ? (ReturnType<PA> extends { meta: infer M }


Just so you know, my eyes are still bleeding here :)

Yeah, that one is wild. But it defines many different behaviours :D

FWIW here's how I understand this code.

PayloadActionCreator always has a type property of type T

If PA is a function we check if it has a meta field. If yes the final type is

{ type: T extends string, meta: M, payload: P }

If the return value does not have a meta field it's

{ type: T extends string, payload: P }

Now the else, if PA is not a function we default back to the older behavior which terrifies me.

@phryneas as some practical feedback, it might be helpful to make it clear to the reader

where the if/else statements are

what the final type will be in each scenario.

Almost, but no.

All Action creators share the { type: T }, but the rest of the functionality differs.

If a PA argument is passed, the first branch will be taken.

Within that branch, if PA returns something with { meta }, again take the first branch. Resulting type is a function (...args: Parameters<PA>) => PayloadAction<P, T, M> (with the additional type from above - all that follow will, too). Otherwise it's a function (...args: Parameters<PA>) => PayloadAction<P, T>).

Now, on to the other branch - no PA given. This has to handle some special cases first and then falls back to the most used case.

If P is undefined, it returns a method that might take 0 or 1 argument. If P is void, it's a method with 0 arguments. Otherwise it is a method with 1 argument. All these methods infer their argument and return that inferred argument as the payload of the returned action.

Formatting this is in a language that knows only ternaries and in a file that will be shuffled around with prettier is unfortunately something I did not achieve :/

markerikson · 2019-07-29T03:50:46Z

src/createSlice.ts

+    : (CR[T] extends (state: any, action: PayloadAction<infer P>) => any
+        ? PayloadActionCreator<P>
+        : CR[T] extends { prepare: PrepareAction<infer P> }
+        ? PayloadActionCreator<P, string, CR[T]['prepare']>


Oh, come on, THAT one isn't even so much different than before =D

Big difference is that it builds the complete ActionCreator type instead of just the payload, and also it now has 4 cases instead of 3.

markerikson · 2019-07-29T03:54:43Z

src/createSlice.ts

+        : PayloadActionCreator<void>)
+}
+
+type NoInfer<T> = [T][T extends any ? 0 : never];


Yeah, I'm still "wat" here, too :)

markerikson · 2019-07-29T03:58:09Z

Welp, I seriously have no "real" feedback to offer on this PR. The type shenanigans y'all are capable of are well and truly beyond me.

But, the examples and the tests look pretty comprehensive, so I'm going to assume this actually works at this point.

Thank you so much for putting this one together! Lots of folks have been asking for this.

markerikson · 2019-07-29T03:58:56Z

Smacking the button!

markerikson mentioned this pull request Jul 11, 2019

createAction doesn't support a meta attribute #148

Closed

phryneas mentioned this pull request Jul 15, 2019

Action creators from createSlice are not inferred as PayloadActionCreator #157

Closed

add prepareAction option to createAction, prepare option to reducers/…

0b3bad1

…slices

phryneas force-pushed the prepareAction branch from ebe366d to 0b3bad1 Compare July 20, 2019 16:20

move logic out of createReducer into createSlice

8ec7395

Fixed typings to raise an error if the type of the payload returned b…

4f55df2

…y prepare and the type accepted by reducer don't agree.

clean up some type tests

5ed1286

markerikson marked this pull request as ready for review July 29, 2019 03:44

markerikson approved these changes Jul 29, 2019

View reviewed changes

markerikson merged commit c033b99 into reduxjs:master Jul 29, 2019

markerikson mentioned this pull request Jul 29, 2019

Using createAction, how would one go about validating the payload passed in? #146

Closed

phryneas mentioned this pull request Jul 26, 2022

State argument on createSlice is no longer inferred with typescript beta 4.8 and they do not plan to fix it #2543

Closed

add prepareAction option to createAction #149

add prepareAction option to createAction #149

Conversation

phryneas commented Jun 22, 2019

netlify bot commented Jun 22, 2019 • edited Loading

markerikson commented Jun 23, 2019 • edited Loading

tanhauhau commented Jun 23, 2019

phryneas commented Jun 23, 2019

phryneas commented Jun 23, 2019 • edited Loading

markerikson commented Jun 23, 2019

phryneas commented Jun 23, 2019 • edited Loading

tanhauhau commented Jun 23, 2019 • edited Loading

markerikson commented Jul 6, 2019

markerikson commented Jul 6, 2019

phryneas commented Jul 6, 2019 • edited Loading

tanhauhau commented Jul 13, 2019

phryneas commented Jul 13, 2019

markerikson commented Jul 13, 2019

phryneas commented Jul 20, 2019 • edited Loading

markerikson commented Jul 20, 2019

phryneas commented Jul 22, 2019 • edited Loading

markerikson commented Jul 22, 2019

markerikson commented Jul 27, 2019

phryneas commented Jul 27, 2019

markerikson commented Jul 27, 2019

phryneas commented Jul 27, 2019

markerikson commented Jul 27, 2019

phryneas commented Jul 28, 2019

markerikson commented Jul 28, 2019 • edited Loading

nickserv commented Jul 28, 2019

markerikson commented Jul 28, 2019

nickserv commented Jul 28, 2019

markerikson commented Jul 28, 2019

nickserv commented Jul 28, 2019 • edited Loading

nickserv commented Jul 28, 2019 • edited Loading

phryneas commented Jul 28, 2019

nickserv commented Jul 28, 2019

phryneas commented Jul 28, 2019

markerikson commented Jul 28, 2019

phryneas commented Jul 28, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markerikson commented Jul 29, 2019

markerikson commented Jul 29, 2019

netlify bot commented Jun 22, 2019 •

edited

Loading

markerikson commented Jun 23, 2019 •

edited

Loading

phryneas commented Jun 23, 2019 •

edited

Loading

phryneas commented Jun 23, 2019 •

edited

Loading

tanhauhau commented Jun 23, 2019 •

edited

Loading

phryneas commented Jul 6, 2019 •

edited

Loading

phryneas commented Jul 20, 2019 •

edited

Loading

phryneas commented Jul 22, 2019 •

edited

Loading

markerikson commented Jul 28, 2019 •

edited

Loading

nickserv commented Jul 28, 2019 •

edited

Loading

nickserv commented Jul 28, 2019 •

edited

Loading