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

Fix typing of `ofType` #289

Merged
merged 1 commit into from Aug 15, 2017

Conversation

@pelotom
Copy link
Contributor

commented Aug 2, 2017

By making the type parameter T extend Action (i.e. telling the compiler that T has a type field),
it's possible to type ofType() correctly using lookup types.

@jayphelps

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

Thanks @pelotom for the PR.

I'm worried it will prevent people from using things that aren't actually actions, like dispatching thunks or other objects that don't have the Action shape--because they're using some other middleware as well. I've seen this in the wild and think it should be supported. Thoughts?

@pelotom

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

My feeling is that I'd rather have a conservative default assumption of what you're dealing with (actions), and if the user knows they might also be dealing with something else they can override this assumption by making an explicit type cast. As opposed to assuming nothing whatsoever and having the default not be type safe. I also think that the majority of the time people are not using any other middleware if they're using redux-observable, and this common case should be what the typings attempt to capture and make safe.

In the absence of this, I think most TypeScript users of redux-observable will just opt not use

action$.ofType('FOO')

preferring the slightly more verbose but type safe

action$.filter(a => a.type === 'FOO')

Then again they might just use

action$selectMany(a => a.type === 'FOO' ? [a] : [])

instead of either, because that provides an even more ideal typing (refining the type parameter of the resulting observable).

@jayphelps

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

Hmm I'm not following? The current usage of generics should provide complete type safety if you set everything up correctly and provide the right type param--if this is not the case, it's a bug in our type definitions.

@pelotom

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2017

@jayphelps how do you figure? The observable's type parameter is not currently used in the parameter types for ofType:

ofType(...key: string[]): ActionsObservable<T>;
ofType(...key: any[]): ActionsObservable<T>;

The key arguments can be any values you want, and don't necessarily have to match the type field of any of the possible actions that could be dispatched. So yes, it's a bug in the type definitions, one this PR is attempting to fix :)

@jayphelps

This comment has been minimized.

Copy link
Member

commented Aug 5, 2017

Sounds like I didn't read this PR closely enough as I didn't notice the ofType T["type"] changes and stuff. Sorry about that. I'll try and fully grasp what you're proposing. 👍

@jayphelps

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

Sorry for the delay--I don't have a ton of OSS time lately as I'm still recovering from surgery. If I don't get back to this within the next week feel free to ping me.

ofType(...key: any[]): ActionsObservable<T>;
lift<R extends Action>(operator: Operator<T, R>): ActionsObservable<R>;
lift<R>(operator: Operator<T, R>): Observable<R>;
ofType(...key: T['type'][]): ActionsObservable<T>;

This comment has been minimized.

Copy link
@philcockfield
@philcockfield

This comment has been minimized.

Copy link

commented Aug 11, 2017

Agreed @pelotom, until ofType can take a type generic, I need to stick with

 action$.filter(a => a.type === 'FOO')

For type safety. It's ugly-er...but the ability to safely rename actions, and other type-safety around the action types, is very desirable.

@philcockfield

This comment has been minimized.

Copy link

commented Aug 11, 2017

I created a similar PR (#288) which does essentially the same thing, but your PR seems to take into account a bunch of wider considerations than mine, so I'd say go with yours @pelotom

ofType(...key: string[]): ActionsObservable<T>;
ofType(...key: any[]): ActionsObservable<T>;
lift<R extends Action>(operator: Operator<T, R>): ActionsObservable<R>;
lift<R>(operator: Operator<T, R>): Observable<R>;

This comment has been minimized.

Copy link
@jayphelps

jayphelps Aug 11, 2017

Member

This should still return and ActionsObservable (the same signature as it was before) or am I confused?

lift<R>(operator: Operator<T, R>): ActionsObservable<R>;

It's not super useful right now since it would only give you chaining ofTypes(), but later we may add additional operators besides ofType. e.g. #281

action$
  .ofType(SOMETHING)
  .debounceTime(200)
  .anotherCustomOperatorWeProvide()

This comment has been minimized.

Copy link
@jayphelps

jayphelps Aug 11, 2017

Member

hmmm thinking about this more, I think I realize why you changed it. It probably won't allow that signature because if the return type is ActionsObservable<R> R must now extend Action but lift<R> does not apply that restriction. i.e. ActionsObservable must always be a stream of actions...which makes sense by its name but its intent was more general. Not sure what the best course is since this isn't a problem that exists today, but likely will at some point 😢

This all may be wrong--I haven't tried it lol.


Edit: one thing we could do is make a base class i.e. ReduxObservable extends Observable. This base class in the future is where we would put generic operators that aren't related specifically to actions. Then ActionsObservable extends ReduxObservable would include those hypothetical future operators as well as adding its ofType.

The so then that generic overload would be:

lift<R>(operator: Operator<T, R>): ReduxObservable<R>;

I think this would be safe to do later, sticking with your Observable<R> for now. Is that right? Since ReduxObservable extends Observable it wouldn't be a breaking change?

This comment has been minimized.

Copy link
@pelotom

pelotom Aug 11, 2017

Author Contributor

Yeah, unfortunately we can't completely constrain the type parameter R of an inherited method. But by providing the 2 overloads

lift<R extends Action>(operator: Operator<T, R>): ActionsObservable<R>;
lift<R>(operator: Operator<T, R>): Observable<R>;

you get the next best thing: whenever you use an Operator<T, R> where R extends Action, it will assume the first signature, so the result type will be ActionsObservable<R>.

In general, if you want to add methods to observables which don't require the assumption that T extends Action, would it not work to augment the definition of Observable itself, similar to how RxJS itself adds its operators piecemeal:

declare module 'rxjs/Observable' {
    interface Observable<T> {
        myObservableOperator: Foo;
    }
}

Then ActionsObservable is free to define overrides that specifically apply when T extends Action constraint, which is what it should be all about, right?

This comment has been minimized.

Copy link
@jayphelps

jayphelps Aug 11, 2017

Member

I don't think we should add them to the Observable prototype because that leaks redux-observable stuff into all rxjs which in practice probably won't be a big deal but is still not a great thing for a library like redux-observable to do (unless of course that's the only point of the library). People also use things we do as proof it's the "right way" so we gotta be careful.

This comment has been minimized.

Copy link
@pelotom

pelotom Aug 11, 2017

Author Contributor

I guess I'm having a failure of imagination... what's an example of a redux-observable-only method which doesn't assume that the value of type of the observable is an action, but which you wouldn't want to be be generally available on all Observables in the app?

This comment has been minimized.

Copy link
@jayphelps

jayphelps Aug 11, 2017

Member

I don't want people to need explicit casts. Is there a problem with my proposal?

Was it this?

Yeah, unfortunately we can't completely constrain the type parameter R of an inherited method.

I didn't understand what you meant by that in our context 🤡 sorry

This comment has been minimized.

Copy link
@pelotom

pelotom Aug 11, 2017

Author Contributor

If you mean this proposal:

This should still return and ActionsObservable (the same signature as it was before) or am I confused?

then yes, the problem is that it's impossible to keep the exact same signature as before if we have ActionsObservable<T extends Action>.

This comment has been minimized.

Copy link
@jayphelps

jayphelps Aug 11, 2017

Member

Hmm maybe you missed my proposal since I added it as an edit: #289 (comment)

This comment has been minimized.

Copy link
@pelotom

pelotom Aug 11, 2017

Author Contributor

I'm not sure what you're proposing there?

This comment has been minimized.

Copy link
@jayphelps

jayphelps Aug 13, 2017

Member

Don't worry about it. I think we can cross that bridge when/if we ever need to.

@pelotom

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2017

I created a similar PR (#288) which does essentially the same thing, but your PR seems to take into account a bunch of wider considerations than mine, so I'd say go with yours @pelotom

Hey @philcockfield, I think the main advantage of this approach is that types involved can be completely inferred without relying on a (possibly faulty, or missing) type annotation. The apparently massive changes introduced with this PR are mostly just fallout from introducing the T extends Action constraint to ActionsObservable.

@jayphelps

This comment has been minimized.

Copy link
Member

commented Aug 13, 2017

Gonna give this a day or two, if no further discussion from community, mergin! 🌮

@jayphelps

This comment has been minimized.

Copy link
Member

commented Aug 13, 2017

@pelotom this will require a rebase when you get a chance--make sure not to blow away the recent changes from #250

@pelotom pelotom force-pushed the pelotom:patch-1 branch from 5eba40c to d0d5f19 Aug 15, 2017

fix(types): Constrain ActionsObservable type param
By making the type parameter `T` extend `Action` (i.e. telling the compiler that `T <: { type: string }`),
it's possible to type `ofType()` correctly using lookup types.

@pelotom pelotom force-pushed the pelotom:patch-1 branch from d0d5f19 to 195e206 Aug 15, 2017

@pelotom

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2017

This should be ready to go now btw

@jayphelps jayphelps merged commit 2144e7d into redux-observable:master Aug 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jayphelps

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

Yay! 🎉 🌮 🌮 🌮 Thanks again for all your work on this!! I'm going to cut a release tomorrow assuming no one reports any issues.

@jayphelps

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

Released as 0.16.0 🎈

@pelotom pelotom deleted the pelotom:patch-1 branch Aug 16, 2017

@pelotom

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2017

Thanks a bunch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.