piotrwitek / typesafe-actions

Typesafe utilities for "action-creators" in Redux / Flux Architecture
https://codesandbox.io/s/github/piotrwitek/typesafe-actions/tree/master/codesandbox
MIT License
2.41k stars 98 forks source link

Rename "ActionsUnion" to "ActionUnion" #38

Closed alecmev closed 6 years ago

alecmev commented 6 years ago

This is nitpicking, but before v2 is completely set in stone, consider maybe renaming ActionsUnion to ActionUnion? I weren't able to find the exact English rule describing this (have no idea how to phrase the search query...), but it just feels more natural, same as "string array" versus "strings array", etc.

Edit

Relevant question on SE.

piotrwitek commented 6 years ago

Thanks for suggestion. I was trying to model that below and perhaps it make sense in some way, but I'm not sure if most people will follow with the same logic as we are:

ActionsUnion = Array<Action1> | Array<Action2> | Array<Action3> ActionUnion = Action1 | Action2 | Action3

EDIT: Why I chosen ActionsUnion is the first place was that it mapped nicely to the typeof actions construct the consumer provides when using it, I was thinking it would be easier to memorize for most people.

alecmev commented 6 years ago

Thanks for replying so quickly. Are you saying that ActionUnion makes more sense to you, but you're afraid that the users of typesafe-actions might misinterpret it? If so, I don't think that's going to confuse anybody (though I'm no authority on this, so, please, do ask around).

In general, I don't think there's a place for ActionsUnion. A better name for it is ActionArrayUnion, i.e. "a union of arrays of actions". Less ambiguous that way, IMO.

Edit

Just noticed the edit.

It does map nicely: "ActionUnion<typeof actions>". But in reverse, "union of actions" is "action union".

piotrwitek commented 6 years ago

@jeremejevs thanks for the discussion I think I'll go even further and rename it to ReturnActionUnion to resemble to ReturnType as it works really similar but on different input object

PS: Because of this discussion I figured out another usefull complementary type helper:

type Action = ReturnActionUnion<typeof actions>;

const rootReducer = combineReducers({
  router: routerReducer,
  counters: countersReducer,
});
type State = ReturnReducerState<typeof rootReducer>;
alecmev commented 6 years ago

Hmm, maybe take it a step further and turn it into a generic ReturnTypeUnion? So, instead of extends ActionCreator just have an extends Function? TypeScript will see if there's a type in the union which is missing a type property, so this doesn't sacrifice the type safety down the line. I think.

piotrwitek commented 6 years ago

I agree it makes sense, but I would still leave an alias to be more specific to the domain, more generic ones would be much better candidates for utility-types package, and I'd move them there

Then I would just import it from utility-types, make alias, and tree-shake to get rid of all unnecessary weight, this would be the best solution I think

alecmev commented 6 years ago

Sounds good!

However, I'm not sure about ReturnActionUnion. I mean, action creators are exactly that, creators of actions, so their return value is Action. So, ActionCreatorReturnTypeUnion is effectively ActionUnion.

piotrwitek commented 6 years ago

Thanks @jeremejevs, your analogy is good, and ActionUnion is a really good candidate, but still have some issues. I spent some time thinking about it and came to following conclusions.

Instead I would propose InferAction. It still conveys a domain concept Action but moreover it also describe it's purpose "to infer".

In addition we could do the same for ReturnReducerState and remain nice consistency in the API by using InferState?

Feedback appreciated.

alecmev commented 6 years ago

Union is a bit of a tautology in this case, I agree.

About plain Action, yeah, probably not a good idea, but how about ActionType? As prior art, React typings are doing something similar with ComponentType, and, well, there's ReturnType, which takes a type as an "argument", and returns a type which could, theoretically, be called Return.

InferAction (or maybe something like ExtractAction) sounds like an alright fallback, though it feels more like a function name, rather than a type name.

Naming is hard... :smile:

piotrwitek commented 6 years ago

Hmm yea, I think ActionType seems like a really good fit considering the following:

I think the best option is to rename them to ActionType and StateType respectively.

Meanwhile I was preparing an update to the docs & tutorial as well, I'll be aiming to release a stable v2.0.0 version of npm package today evening with above changes included.

PS: @jeremejevs thanks a lot for your valuable input on this matter 👍

alecmev commented 6 years ago

Awesome, thanks again, for the responsiveness and the library :slightly_smiling_face: I'll leave the closure of this to you.