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(store.dispatch): Calling store.dispatch() directly in your Epic is now deprecated #346

Merged
merged 1 commit into from Oct 31, 2017

Conversation

jayphelps
Copy link
Member

@jayphelps jayphelps commented Oct 30, 2017

This is unrelated to usage of store.dispatch inside your UI components or anywhere outside of redux-observable--you will continue to use it there

The ability to call store.dispatch() inside your Epics was originally provided as an escape hatch, to be used rarely, if ever. Unfortunately in practice we've seen a large number of people using it extensively; there has even been popular tutorials teaching it as how you use redux-observable. This is ultimately my fault as providing this ability first-class made it seem like it was sanctioned. Instead, Epics should emit actions through the Observable the Epic returns, using idiomatic RxJS. If you're looking for the ability to directly call dispatch yourself (rather than emit through streams) take a look at some of the other middleware available in the community. 🥇

Before

const somethingEpic = (action$, store) =>
  action$.ofType(SOMETHING)
    .switchMap(() =>
      ajax('/something')
        .do(() => store.dispatch({ type: SOMETHING_ELSE }))
        .map(response => ({ type: SUCCESS, response }))
    );

After

const somethingEpic = action$ =>
  action$.ofType(SOMETHING)
    .switchMap(() =>
      ajax('/something')
        .mergeMap(response => Observable.of(
          { type: SOMETHING_ELSE },
          { type: SUCCESS, response }
        ))
    );

store.dispatch will be removed from Epics in v1.0.0 of redux-observable alongside the removal of store.getState() but replaced with a observable of state$ that also has state$.value if want to just access the current value.

Cc/ @evertbouw

@@ -37,7 +37,7 @@ export function createEpicMiddleware(rootEpic, options = defaultOptions) {
const vault = (process.env.NODE_ENV === 'production') ? store : {
getState: store.getState,
dispatch: (action) => {
console.warn(`Your Epic "${epic.name || '<anonymous>'}" called store.dispatch directly. This is an anti-pattern.`);
require('./utils/console').deprecate('calling store.dispatch() directly in your Epics is deprecated and will be removed. Instead, emit actions through the Observable your Epic returns.\n\n https://goo.gl/WWNYSP');
Copy link
Member Author

Choose a reason for hiding this comment

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

This is so that when process.env.NODE_ENV === 'production' we don't import the console.js file at all since it's not needed.

Copy link
Member

Choose a reason for hiding this comment

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

I like this pattern.

if (!deprecationsSeen[msg]) {
deprecationsSeen[msg] = true;
console.warn(`redux-observable | DEPRECATION: ${msg}`);
}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for making it back off after being logged at least once.

@@ -202,7 +202,7 @@ dispatch(incrementIfOdd());
```
> REMEMBER: When an Epic receives an action, it has already been run through your reducers and the state updated.

Using `store.dispatch()` inside your Epic is a handy escape hatch for quick hacks, but use it sparingly. It's considered an anti-pattern and we may remove it from future releases.
Using `store.dispatch()` inside your Epic is an anti-pattern and will be removed in v1.0.0 of redux-observable so it's best not to use it.
Copy link
Member

Choose a reason for hiding this comment

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

I like how this goes from handy escape hatch to anti-pattern.

…s is now deprecated

The ability to call `store.dispatch()` inside your Epics was originally provided as an escape hatch, to be used rarely, if ever. Unfortunately in practice we've seen a large number of people using it extensively; there has even been popular tutorials teaching it as how you use redux-observable. Instead, Epics should emit actions through the Observable the Epic returns, using idiomatic RxJS.

```js
const somethingEpic = (action$, store) =>
  action$.ofType(SOMETHING)
    .switchMap(() =>
      ajax('/something')
        .do(() => store.dispatch({ type: SOMETHING_ELSE }))
        .map(response => ({ type: SUCCESS, response }))
    );
```

```js
const somethingEpic = action$ =>
  action$.ofType(SOMETHING)
    .switchMap(() =>
      ajax('/something')
        .mergeMap(response => Observable.of(
          { type: SOMETHING_ELSE },
          { type: SUCCESS, response }
        ))
    );
```

`store.dispatch` will be removed from Epics in v1.0.0 of redux-observable. This is unrelated to usage of `store.dispatch` inside your UI components--you will continue to use it there
@jayphelps
Copy link
Member Author

Ok once the build passes we're good to go.

@jayphelps jayphelps merged commit a1ba6a2 into redux-observable:master Oct 31, 2017
@jayphelps
Copy link
Member Author

Thanks for the review @rgbkrk!!

@adred
Copy link

adred commented Nov 29, 2017

I'm new to rxjs. So does this mean I won't be able to call action creators anymore? It sucks because instead of reusing action creators I now have to type the actions. :-(

@jayphelps
Copy link
Member Author

jayphelps commented Nov 29, 2017

@adred fear not! you can ABSOLUTELY use action creators. This only deprecated using the store.dispatch function inside epics, that’s it.

@adred
Copy link

adred commented Nov 30, 2017

@jayphelps yeah, that's what I meant. I'm using dispatch(actionCreator) inside my epics.

@jayphelps
Copy link
Member Author

@adred gotcha, different terminology.

It’s very anti redux-observable and was never intended to be used except for hacks—which a person can still do, they can inject the store themselves, or export and import it, etc. I recommend against it though, as this is fighting the redux-observable way. There’s no harm in not liking it, there are lots of middleware to choose from or you can roll your own. 🖖

@rgbkrk
Copy link
Member

rgbkrk commented Nov 30, 2017

If you need to dispatch in the middle of your epic, there are many ways to do it. By the nature of the epic you need to emit actions anyways. You can merge together multiple streams of actions for example using mergeMap:

import { of } from "rxjs/observable/of";
import { empty } from "rxjs/observable/empty";
import { mergeMap } from "rxjs/operators";

action$.ofType("SOME_TYPE").pipe(
  mergeMap(action => {
    // within merge map you return an observable of actions
    // and they become the next part of the stream
    if(...) {
      // You can return one
      return of(someAction);
    } else if (...) {
      // or many
      return of(someAction, anotherAction);
    } else {
      // or nothing
      return empty();
    }
   // or even whole new streams composed of various things
  })
)

How you approach it depends on what kind of control flow you want.

@trubi
Copy link

trubi commented Dec 6, 2017

Guys, quick question.
Sometimes I am using this (with native module):

const magtekModule = NativeModules.MagtekManager
const eventEmitter = new NativeEventEmitter(magtekModule)

eventEmitter.addListener(
    "MagtekManagerEventDataReady",
    (payload) => dispatch({
            type: MAGTEK_MANAGER_DATA_READY,
            payload: {
                magstripe: payload[0],
        }
    }))

Or what else should I use if I need to dispatch action from outside of my redux app? Some kind of middleware? Could you point me somewhere? Thanks!

@jayphelps
Copy link
Member Author

jayphelps commented Dec 6, 2017

@trubi happy to help if you wanna ask it on StackOverflow so the knowledge is shared.

@trubi
Copy link

trubi commented Dec 6, 2017

@ecnaidar
Copy link

ecnaidar commented Mar 7, 2018

So the getState() will still be available as a parameter, right? Removing dispatch is a sane move to make people do thing Rx way, but removing ability to access state might make some things unnecessarily complicated.
I will leave what I have came up with here just in case. No idea if that is a 100% correct way of dispatching in the middle of epic. (Not really experienced with RxJS but worked for a while with redux-saga)

action$
    .ofType(CREATE)
    .flatMap(action => {
      // Save our request observable so we can process output two different ways
      const request$ = Observable.fromPromise(requestCreate(action.payload));
      return [
        // Before API request 
        Observable.of(startSubmit("template")),
        // 1. Notify that load is done
        request$.map(response => {
          console.log("stopping submit");
          return stopSubmit("template", response.error || {});
        }),
        // 2. Format and emit result
        request$
          .map(response => {
            console.log("returning response");
            console.log(response);
            if (!response.errors) {
              return saveTemplate(response.data);
            } else {
              return undefined;
            }
          })
          .filter(v => v)
      ];
      // Each element produces plain redux action
    })
    .concatAll();

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

5 participants