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

Type inference for ofType, removal of ActionsObservable in favor of just Observable #681

Merged
merged 1 commit into from Nov 14, 2019

Conversation

jayphelps
Copy link
Member

@jayphelps jayphelps commented Oct 19, 2019

Heavily inspired by examples that @thorn0 @tjfryan did in #622


For v2.0.0 I'd like to finally have ofType infer the TypeScript types automatically, so that you don't need to pass in the generic param or define your filter action(s) as the argument type of the next operator. This requires some voodoo and a new-ish version of TypeScript, but I'm not yet sure which minimum version. I'll have to try them until it breaks, before merging this, then I can document it in the changelog/docs.

At the same time, I want to get away from ActionsObservable because in the pipeable operator world, there's no need for it and it just adds friction/learning.

The primary reason StateObservable is not BehaviorSubject is because we want to expose state$.value but _we do not want to expose state$.next(something) because that would be a huge footgun that wouldn't do what you might think--no reason to do it inside your epics.


Have objections or other thoughts? Please lmk! We'll likely do the normal pre-alpha, alpha, beta, rc, etc cycle for 2.0, but depending on how smooth it goes, how many changes there are (don't expect too many) we might skip a step or two of those.

Copy link

@cartant cartant left a comment

LGTM

@kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Oct 19, 2019

🚢

@evertbouw
Copy link
Member

@evertbouw evertbouw commented Oct 19, 2019

I don't think it's necessary to deprecate ActionObservable before removing it. Changing the ofType method to an imported function is not a huge change and probably the only thing users will have to do, and they may have already done it.

@@ -42,7 +41,7 @@ export function createEpicMiddleware<T extends Action, O extends T = T, S = void
const stateSubject$ = new Subject<S>().pipe(
observeOn(uniqueQueueScheduler)
) as any as Subject<S>;
Copy link
Contributor

@csvn csvn Oct 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: as any as Subject<S> can be removed, now that a regular Observable is used instead of ActionsObservable.

I love this PR, as I think it simplifies both usage and the code quite a bit. Sorry if I just jumped in with this review.

Copy link
Member Author

@jayphelps jayphelps Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful! Thank you for pointing this out.

Copy link
Member Author

@jayphelps jayphelps Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it couldn't just be removed as-is because while observeOn() uses RxJS's internal lift() function so it does return a Subject, the type definition does not reflect that with pipe() so from the type system point of view it's just an Observable and doesn't have .next() nor .asObservable(). No biggie as I was able to refactor the code in another way to avoid the casts.

…ble in favor of just Observable

BREAKING CHANGE: ActionsObservable existed so we could provide an ofType() method to the prototype of action$, before RxJS had pipeable operators. Now that pipeable operators have been out for quite some time we are removing ActionsObservable in favor or using the pipeable ofType() instead.

```js
// BEFORE
function someEpic(action$) {
  return action$
    .ofType('PING')
    .mapTo({ type: 'PONG' });
}

// AFTER
import { ofType } from 'redux-observable';
import { mapTo } from 'rxjs/operators';
function someEpic(action$) {
  return action$.pipe(
    ofType('PING')
    mapTo({ type: 'PONG' })
  );
}
```
@jayphelps jayphelps changed the title WIP: Type inference for ofType, removal of ActionsObservable in favor of just Observable Type inference for ofType, removal of ActionsObservable in favor of just Observable Nov 14, 2019
@jayphelps jayphelps merged commit 16f083d into master Nov 14, 2019
2 checks passed
@jayphelps
Copy link
Member Author

@jayphelps jayphelps commented Nov 15, 2019

Thanks for all the reviews! I've cut a pre-release 2.0.0-alpha.0 or the next tag (which will be updated to whatever the latest pre-release is.

@csvn
Copy link
Contributor

@csvn csvn commented Dec 4, 2019

@jayphelps I noticed the release commit 9a85f64, but no publish seems to have been done to NPM (https://www.npmjs.com/package/redux-observable?activeTab=versions). Maybe there's some issue with the publishing pipeline. Should I create an issue for this?

@jayphelps
Copy link
Member Author

@jayphelps jayphelps commented Dec 4, 2019

Whoa. I know 100% for sure the 2.0 alpha was released and tagged as next. It’s been installed and used in several apps that I’m aware of. Not sure what happened. I’ll look into it ASAP likely this weekend. Thanks for the heads up! Feel free to make a ticket if you’d like to subscribe to updates

@jayphelps
Copy link
Member Author

@jayphelps jayphelps commented Dec 4, 2019

Proof I’m not crazy.
A370ED88-F3F5-4D76-9D86-142AB2CF758E

@evertbouw
Copy link
Member

@evertbouw evertbouw commented Dec 4, 2019

npm info redux-observable shows

dist-tags:
latest: 1.2.0        next: 2.0.0-alpha.0  

@jayphelps
Copy link
Member Author

@jayphelps jayphelps commented Dec 4, 2019

Phew! Thanks for checking that @evertbouw it must just be a UI bug or DB issue. I’m not able to try to install it right now, but anyone should let us know if you can’t install.

@csvn
Copy link
Contributor

@csvn csvn commented Dec 4, 2019

That's weird... I didn't run npm info redux-observable, I just checked the NPM page, but it's still not showing the new alpha version 🤔

image

Thanks a lot, sorry for the noise!

edit: Running the npm command shows latest: 1.2.0 next: 2.0.0-alpha.0 as dist tags correctly
edit2: ...and npm install redux-observable@next works

@israelidanny
Copy link

@israelidanny israelidanny commented May 7, 2020

I'm trying to play around with this but can't seem to get the type inference to work. Where do I supply my actions union type for the inference to work?

I have something like this:

// Action is my union type of all possible action types for the app
export const loginEpic: Epic<Action, Action, RootState> = (action$) =>
  action$.pipe(
    ofType("LOGIN_REQUEST"),
    mergeMap((action) =>
      ajax({
        url: `${API_URL}/auth/login`,
        method: "POST",
        headers: {
          "Content-Type": "application/json",
        },
        body: {
          // I end up getting an error here, because the type gets incorrectly inferred
          username: action.payload.username,
          password: action.payload.password,
        },
      }).pipe(
        map((response) => loginSuccess(response.response.token)),
        catchError((error) => of(loginFailure(error.message))),
      ),
    ),
  );

@jayphelps
Copy link
Member Author

@jayphelps jayphelps commented May 8, 2020

@israelidanny Unfortunately this didn't actually make ofType infer the types correctly. I swear it worked when I PRd it, but apparently not. 😢 I haven't had time to prioritize revisiting this as I'm not working on any redux-observable codebases at the moment. If someone can get it to work, I'd super appreciate any suggestions or even a PR if they're feeling ambitious. Otherwise I'll for sure eventually get to it, but can't say when.

@kwonoj
Copy link
Contributor

@kwonoj kwonoj commented May 8, 2020

Coincidentally I just tried to update my code to use this and hitting same thing, thought I was doing something wrong. 😭

@csvn
Copy link
Contributor

@csvn csvn commented May 11, 2020

@jayphelps @kwonoj @israelidanny We are able to use type inference in our code for ofType, but it requires some thought. What I think the issue mentioned above is hitting is that Typescript is widening the types. For example, 'LOGIN_REQUEST' becomes string instead of the literal type 'LOGIN_REQUEST'.

The below is a working example of inference for ofType by using as const to avoid Typescript widening the type from the specific enum value to ActionType (1). The example works the same if replacing the enum with hard-coded string values.

import { Epic, ofType } from 'redux-observable';
import { tap, ignoreElements } from 'rxjs/operators';


enum ActionType {
  Update = 'COUNT_UPDATE',
  Reset = 'COUNT_RESET'
}

type State = any;
type Action = ReturnType<typeof setCount> | ReturnType<typeof resetCount>;

const setCount = (count: number) => ({
  type: ActionType.Update as const, // 1
  payload: count
});
const resetCount = () => ({
  type: ActionType.Reset as const // 1
});

export const testEpic: Epic<Action, Action, State, undefined> = action$ => action$.pipe(
  ofType(ActionType.Update as const), // 1
  // `payload` is correctly inferred to be `number`
  tap(({ payload }) => console.log(payload + 1)),
  ignoreElements()
);

It might be possible to improve the typing so you don't need ofType('<TYPE>' as const)`, but I have no idea where to start. I find some of the more intricate matters with generics and Typescript a bit confusing 😄

sandinosaso pushed a commit to sandinosaso/redux-observable that referenced this issue Oct 9, 2020
…ble in favor of just Observable (redux-observable#681)

BREAKING CHANGE: ActionsObservable existed so we could provide an ofType() method to the prototype of action$, before RxJS had pipeable operators. Now that pipeable operators have been out for quite some time we are removing ActionsObservable in favor or using the pipeable ofType() instead.

```js
// BEFORE
function someEpic(action$) {
  return action$
    .ofType('PING')
    .mapTo({ type: 'PONG' });
}

// AFTER
import { ofType } from 'redux-observable';
import { mapTo } from 'rxjs/operators';
function someEpic(action$) {
  return action$.pipe(
    ofType('PING')
    mapTo({ type: 'PONG' })
  );
}
```
@MaximeBernard MaximeBernard deleted the oftype branch May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants