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

Update TypeScript typings for more common use cases #446

Closed
ajcrites opened this Issue Mar 9, 2018 · 4 comments

Comments

Projects
None yet
5 participants
@ajcrites
Contributor

ajcrites commented Mar 9, 2018

Do you want to request a feature or report a bug?
I think this could be considered either, but I don't know the original philosophy of the typings.

This can possibly be broken out into multiple issues, but I think they are all related.

What is the current behavior?

Epic output action extends trigger action

interface Epic<T extends Action, S, D = any, O extends T = T> where O extends T. This means that any action emitted by the Epic has to conform to the structure of the action that triggers the epic. I think that it is very common to emit actions that have different properties. For example:

interface LoadUserAction {
  type: 'LOAD_USER';
  username: string;
}

interface LoadUserCompleteAction {
  type: 'LOAD_COMPLETE_USER';
  user: User;
}

const loadUserEpic: Epic<LoadUserAction, never, never, LoadUserCompleteAction>

This is not allowed because 'username' doesn't exist on LoadUserCompleteAction.

Update Epic generic type order for more common uses

It's common to have an epic that doesn't rely on the store at all, but if you want to add the generic type for trigger actions you have to set a type for the store state because it doesn't have a default.

I also don't think that options are commonly used, and in fact they're not even documented or properly typed (defaulting to any).

This leads me to use a lot of the following:

const epic: Epic<InputActions  | OutputActions, never, never, InputActions | OutputActions>

combineEpics doesn't work well with epics using different types

The combineEpics generic typings only make sense if all of the Epics are of the same type, but this is uncommon -- at least compared to combining epics of different types.

const epic1: Epic<Epic1Trigger | Epic1Output, never>
const epic2: Epic<Epic2Trigger | Epic2Output, never>

const epics = combineEpics(epic1, epic2);

This is not allowed because the epic1 and epic2 actions do not conform to each others' types. To get around this you can circumvent type safety:

combineEpics<any>
combineEpics(...[epic1, epic2]);

or you have to be verbose

combineEpics<Epic1Trigger | Epic1Output | Epic2Trigger | Epic2Output, never>
combineEpics<typeof epic1 | typeof epic2>

What is the expected behavior?

I have some suggested solutions, and I think that the proper solution should be to consider the most beneficial combination of ergonomics, type safety, and code completion.

  • Epic output action:
    Have O extend Action. It can default to T which keeps the same behavior as the current typings if it's not provided.
interface Epic<T extends Action, S, D = any, O extends Action = T>
  • Optional Store
    All epics must be triggered by an action and emit an action, so these types are required -- however, the output action could be the same type as the trigger action (see above). Thus I think that the Epic generic types should be reordered according to use case. Using the store is less common, and using options is the least common.
interface Epic<T extends Action, O extends Action = T, S = never, D = any>

I have S = never so that store state cannot be accessed without explicit typing. Ideally it would be able to get the store state from the middleware created by createStore, but I think that would be impossible.

D should probably be of a specific type ... something like interface Options { dependencies; adapter; }; ... D extends Options = {}>

  • combineEpics issues
    Due to limitations of TypeScript, I don't think that we can make combineEpics type safe appropriately right now. Perhaps once variadic kinds are implemented.

I don't think that generic types for combineEpics offer any benefit, so combineEpics(...epics: any[]) could be added. We can still keep the other type signatures to allow for more explicit / verbose typing for people who want to use them.


If this is none of these are the desired outcome for typings, I think this should be documented -- at least with examples.

Which versions of redux-observable, and which browser and OS are affected by this issue? Did this work in previous versions of redux-observable?

v0.18.0 -- earlier typings would have had similar issues.

@bsunderhus

This comment has been minimized.

bsunderhus commented Mar 10, 2018

I just passed through this case:

interface Epic<T extends Action, S, D = any, O extends Action = T>

Would be useful to have a more generic case to Epic declaration

@cjol

This comment has been minimized.

Contributor

cjol commented Apr 5, 2018

The way I use the type variables is to set T as the union of all actions in use in my application. That way, your example would look like

type RootAction = LoadUserAction | LoadUserCompleteAction
const loadUserEpic: Epic<RootAction, never, never, LoadUserCompleteAction>

but the reality is that most consumers of loadUserEpic don't really care what the output actions are - just that they are some kind of Action. For this case, the current default typings work fine: const loadUserEpic: Epic<RootAction>.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Apr 9, 2018

The biggest reason for the current generic type order was minimizing breaking changes, BUT now is the the time if we want them, before 1.0.0-final. I have no objection to reordering.

The reason the output extends the input is because technically speaking every action that is output is always recursively provided as the input, it's just usually filtered out by ofType. As a bit of a purist I prefer to have things as accurate as possible because while sometimes a pain it could potentially prevent mistakes. e.g. if you don't provide your output actions as inputs you could potentially make a mistake in your custom filtering logic, though I'll admit it's not likely to be an issue for most people in practice.

In this instance I mostly just want to do what the community wants, so everyone please voice opinions! @ajcrites do you mind putting together the PR?

ajcrites added a commit to ajcrites/redux-observable that referenced this issue Apr 10, 2018

feat(typings): Updating typings for more common use-cases.
This is a first draft / suggestion for a possible reordering of typings re: redux-observable#446 that accomplishes the following goals:

* Generic types for Epics are reordered to InputActions, OutputActions, State, Dependencies
* OutputActions now extend from Action instead of T allowing for more specific declarations for input and output actions
* State is made optional and defaults to 'void' for cases where it would be unused. This requires an explicit type for the generic if it's to be used
* Added a type for combineEpics that takes no generic arguments. It is common to have epics with different type signatures, and it would be very difficult / impossible to enforce type safety through generics for combineEpics at this level.

Tests have also been updated to use the new generic type ordering
@evertbouw

This comment has been minimized.

Member

evertbouw commented May 25, 2018

#473 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment