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

add prepareAction option to createAction #149

Merged
merged 4 commits into from Jul 29, 2019

Conversation

@phryneas
Copy link
Contributor

phryneas commented Jun 22, 2019

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

This comment has been minimized.

Copy link

netlify bot commented Jun 22, 2019

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

Built with commit 5ed1286

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

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jun 23, 2019

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

This comment has been minimized.

Copy link

tanhauhau commented Jun 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jun 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jun 23, 2019

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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jun 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jun 23, 2019

@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

This comment has been minimized.

Copy link

tanhauhau commented Jun 23, 2019

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

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 6, 2019

Anyone had a chance to think through this further?

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 6, 2019

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

This comment has been minimized.

Copy link

tanhauhau commented Jul 13, 2019

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

@phryneas

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 13, 2019

@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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 13, 2019

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

@phryneas phryneas force-pushed the phryneas:prepareAction branch from ebe366d to 0b3bad1 Jul 20, 2019
@phryneas

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 20, 2019

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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 20, 2019

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

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 22, 2019

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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 22, 2019

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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 27, 2019

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

@phryneas

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 27, 2019

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

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 27, 2019

No worries, just checking!

@phryneas

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 27, 2019

@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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 27, 2019

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

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 28, 2019

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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 28, 2019

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

@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Jul 28, 2019

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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 28, 2019

@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.

@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Jul 28, 2019

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

This comment has been minimized.

@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Jul 28, 2019

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> = {

@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Jul 28, 2019

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

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 28, 2019

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?

@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Jul 28, 2019

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

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 28, 2019

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 28, 2019

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

This comment has been minimized.

Copy link
Contributor Author

phryneas commented Jul 28, 2019

No hurry :)

@markerikson markerikson marked this pull request as ready for review Jul 29, 2019
> = {
type: T
} & (PA extends (...args: any[]) => any
? (ReturnType<PA> extends { meta: infer M }

This comment has been minimized.

Copy link
@markerikson

markerikson Jul 29, 2019

Collaborator

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

This comment has been minimized.

Copy link
@phryneas

phryneas Jul 29, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@matthew-gerstman

matthew-gerstman Jul 30, 2019

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

  1. where the if/else statements are
  2. what the final type will be in each scenario.

This comment has been minimized.

Copy link
@phryneas

phryneas Jul 30, 2019

Author Contributor

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 :/

: (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']>

This comment has been minimized.

Copy link
@markerikson

markerikson Jul 29, 2019

Collaborator

wat

This comment has been minimized.

Copy link
@phryneas

phryneas Jul 29, 2019

Author Contributor

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.

: PayloadActionCreator<void>)
}

type NoInfer<T> = [T][T extends any ? 0 : never];

This comment has been minimized.

Copy link
@markerikson

markerikson Jul 29, 2019

Collaborator

voodoo

This comment has been minimized.

Copy link
@phryneas

phryneas Jul 29, 2019

Author Contributor

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

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 29, 2019

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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jul 29, 2019

Smacking the button!

@markerikson markerikson merged commit c033b99 into reduxjs:master Jul 29, 2019
6 checks passed
6 checks passed
Header rules - redux-starter-kit-docs No header rules processed
Details
Pages changed - redux-starter-kit-docs All files already uploaded
Details
Redirect rules - redux-starter-kit-docs No redirect rules processed
Details
Mixed content - redux-starter-kit-docs No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/redux-starter-kit-docs/deploy-preview Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.