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

feat(replaceEpic): Added middleware method to replace the root Epic. #75

Merged
merged 1 commit into from Jul 24, 2016

Conversation

Projects
None yet
2 participants
@jayphelps
Member

jayphelps commented Jul 16, 2016

NOTE: I accidentally pushed an older version of this to master days ago so #YOLO....part of this has already shipped. We can always revert, so it's not set in stone though.

I'm still not convinced that using this is 100% safe, since Epics may contain forms of state. There may be edge cases where losing that state causes the app to be in a bad state. For the most part that sort of state belongs in the store anyways, but this is likely not always obvious and there may be cases of ephemeral state that should be in your Epic; after all, things like Observable.ajax() maintains internal state while the request is in-flight. Thinking we'll release it anyway and find out in practice whether it's an issue or not and then advise appropriately or rethink.

AFAICT redux-saga doesn't support Hot reloading either for basically the same reasons redux-saga/redux-saga#22 Correct me if I'm wrong on this--the demos don't show it being used either, only for reducers.

Keep in mind that the old observables will all be unsubscribed, so you can use that opportunity to clean up any ephemeral state--things like Observable.ajax() will clean themselves up, cancelling in-flight AJAX requests. The .finally() operator is useful for this.

Closes #52, #53, #73

import rootEpic from './where-ever-they-are';
const epicMiddleware = createEpicMiddleware(rootEpic);

if (module.hot) {
  module.hot.accept('./where-ever-they-are', () => {
    const rootEpic = require('./where-ever-the-are').default;
    epicMiddleware.replaceEpic(rootEpic);
  });
}

@jayphelps jayphelps force-pushed the replace-epic branch 4 times, most recently from 4416891 to 0e32d7e Jul 16, 2016

@ronag

This comment has been minimized.

ronag commented Jul 16, 2016

What if replaceEpic sent an EOFaction, disconnected and buffered action$ and used concatMap instead of switchMap. That would allow the epic to be placed into a valid state before being replaced. Would solve e.g. debounce cases.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jul 16, 2016

Side note, AFAICT redux-saga doesn't support Hot reloading either for basically the same reasons redux-saga/redux-saga#22 Correct me if I'm wrong on this--the demos don't show it being used either, only for reducers.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jul 16, 2016

What if replaceEpic send an EOF action and used concatMap instead of switchMap. That would allow the epic to be placed into a valid state before being replaced. Would solve e.g. debounce cases.

@ronag But then you're explicitly requiring all Epics takeUntil otherwise you'd duplicate Epics listening. That seems like a poor trade off to handle edge cases where replacing them might be a problem. Disagree?

@ronag

This comment has been minimized.

ronag commented Jul 16, 2016

@jayphelps: Not necessarily. An EOF action might not even be required as long as epics are guaranteed to eventually end if action$ completes.

So replaceEpic should complete the current action$, create a new stream from the new epic and concatMap map it on the old stream.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jul 16, 2016

@ronag I'm still very hesitant. I can think of several cases in one of my app's codebase where the stream would not end because it's not derived from action$. I'd rather the default is everything you give me is completed, even if not derived from action$, as the new series are switched in. If someone wants to signal a shutdown, they certainly can before they replaceEpic.

store.dispatch({ type: 'END' });
epicMiddleware.replaceEpic(nextEpic);

It might be notable to mention that replaceEpic won't just terminate your streams abruptly--it simply unsubscribes from the observable you gave it from your previous epic, so RxJS will run .finally() and any operators that have unsubscribe() handlers will be invoked, including any custom observables you create a la Observable.create(({ next }) => { next(1); return () => { /* cleanup */ }; })

I feel this is the conservative approach, but I'm not writing off your suggestion.

Let's figure out some concrete cases where additional clean up is required. The debounceTime example I don't personally think is a big deal and arguably desired because the replacement epic may have different behavior.

@ronag

This comment has been minimized.

ronag commented Jul 17, 2016

const epic = action$ => action$
  .ofType('SEARCH')
  .debounceTime(1000)
  .switchMap(x => API.search(x)
    .then(result => ({ type: 'SEARCH_SUCCESS' }))
    .catch(result => ({ type: 'SEARCH_FAILED' }))
  )

const reducer = handleActions({
  ['SEARCH']: (state, action) => ({
    ...state,
    isSearching: true
  }),
  ['SEARCH_SUCCESS']: (state, action) => ({
    ...state,
    isSearching: false
  }),
  ['SEARCH_FAILED']: (state, action) => ({
    ...state,
    isSearching: false
  })
})

Would never get the search result, i.e. either FAILURE or SUCCESS and therefore the state isSearching would be invalid and the reducer is broken.

It's not just about cleanup. It's about maintaining state invariants which is only possible if the epic is allowed to properly end.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jul 18, 2016

@ronag

Absolutely. I was thinking we'd advise in the documentation about this and recommend they dispatch some action as a signal--but perhaps we should indeed just dispatch one automatically? Is that what you were thinking?

import { race } from 'rxjs/observable/race';

const epic = action$ =>
  action$.ofType('SEARCH')
    .debounceTime(1000)
    .switchMap(x => race(
      API.search(x)
        .then(result => ({ type: 'SEARCH_SUCCESS' }))
        .catch(result => ({ type: 'SEARCH_FAILED' })),
      action$.ofType('END')
        .mapTo({ type: 'SEARCH_CANCELLED' })
    ));


// later...
store.dispatch({ type: 'END' });
epicMiddleware.replaceEpic(nextEpic);

Anyone else have opinions or suggestions?

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jul 18, 2016

FWIW redux-saga uses the END action as a signal to shutdown, used for SSR and (presumably) can be used for hot reloading.

This release introduces a special action END which can be used to notify that an event source has terminated. You can also dispatch an END to notify Sagas that no more Store actions will be fired. This can be handy in Server Side rendering to break the while(true) loop inside watchers.The real-world example in the repo shows a possible ways to implement universal Sagas and SSR.

END actions are automatically handled by the middleware and cause a Saga blocked on a take effect to automatically terminate (but it'll still wait for its forked tasks which provides support for SSR). To catch END values you can use the new Effect takem (aka takeMaybe). Using takem you get the explicit END so you can handle it manually. See #255 for more infos

If we do emit something similar for you automatically on replace, then we prolly want to make it unique in some way to not conflict with people's existing action types and not prevent people from using redux-saga along side redux-observable; which certainly could happen as they transition from either one. So perhaps "@@EPIC_END" and we can export it as the variable EPIC_END or something.

@ronag

This comment has been minimized.

ronag commented Jul 18, 2016

@@EPIC_END already exists in rxjs, complete I don't think any magical token is required.

const epic = action$ =>
  action$.ofType('SEARCH')
    .debounceTime(1000)
    .concat(action$.ofType('SEARCH').last())
    .switchMap(x => 
      API.search(x)
        .then(result => ({ type: 'SEARCH_SUCCESS' }))
        .catch(result => ({ type: 'SEARCH_FAILED' }))
    )


// later...
epicMiddleware.replaceEpic(nextEpic);

replaceEpic() {
  action$.complete()
  action$ = new Subject()
  epic$ = epic£.concatMap(action$ ...
  // ...
}
@jayphelps

This comment has been minimized.

Member

jayphelps commented Jul 18, 2016

I'm not following. You example appears to still leave the store in a bad isSearching: true state. The reason I was suggesting an action to proceed completion is it's the only way you can emit another action to update the store to a good state.

@ronag

This comment has been minimized.

ronag commented Jul 18, 2016

@jayphelps: Sorry, I forgot debounceTime doesn't send the last item. I fixed it now.

You shouldn't need another action. This is the explicit purpose of complete.

@jayphelps jayphelps force-pushed the replace-epic branch from 0e32d7e to 886656c Jul 19, 2016

feat(replaceEpic): Dispatches an EPIC_END action when you replaceEpic()
so that you can listen for it and emit any state cleanup

@jayphelps jayphelps force-pushed the replace-epic branch from 886656c to 334f160 Jul 19, 2016

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jul 19, 2016

@ronag I discussed this with a couple others who have been using redux-observable heavily at Netflix (including @blesh, who wrote most of RxJS v5) and they too believe completing the ActionsObservable and concatMap instead of switchMap isn't a safe solution. The biggest issue is that this does not actually cancel anything that isn't derived directly from the ActionsObservable. That includes anything you switchMap/flatMap against an action. This is probably a bit counter-intuitive, but can be easily confirmed:

http://jsbin.com/gevijit/edit?js,output

const subject = new Subject();
const action$ = new ActionsObservable(subject);

const intervalEpic = action$ =>
  action$.ofType('START_INTERVAL')
    .switchMap(() =>
      Observable.interval(1000)
        .mapTo({ type: 'INTERVAL' })
    );

intervalEpic(action$).subscribe(value => {
  console.log(value);
});

subject.next({ type: 'START_INTERVAL' });
// Observable.interval() does not derive from actions$
// so this will not stop it from emitting
subject.complete();

We've tested our the automatic emitting of a "end" action that you can listen for either in your Epics or obviously in your reducers as well. It seems to work well so far..but too early to know how good of solution it is in practice. We're thinking we'll merge that, let it settle in the community a bit, and then pivot if needed before v1.0. I've updated this PR with that code.

What are your thoughts on the issues I described to the actions.complete() solution and the proposed alternative?

Ultimately if someone wants to avoid this issue, they can avoid hot reloading (let normal full reloads act) and for async routes they can use pretty simple Rx to add new Epics without terminating the old ones:

http://jsbin.com/tuxikuk/edit?js,output

const epic$ = new BehaviorSubject(someEpic);
const rootEpic = (action$, store) =>
  epic$.mergeMap(epic =>
    epic(action$, store)
  );

// sometime later...add another Epic, keeping the state of the old ones...
epic$.next(asyncEpic);

We might provide an API to do this like runEpic or addEpic but I'd prefer not to..I like keeping the middleware only deals with a single Epic and it's up to you to combine or merge how you wish.

None of this is "ideal" because dealing with side effects is not easy. as an aside, I'm truly surprised this isn't a bigger issue for redux-saga users historically. I'm guessing the answer is just that in practice it's not often a big deal because hot reloading/async route loading usually isn't done mid-UI interaction where state would be changing..also very few people load routes async.

@jayphelps jayphelps merged commit fef6f80 into master Jul 24, 2016

2 checks passed

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

This comment has been minimized.

Member

jayphelps commented Jul 24, 2016

:shipit: If this ends up being bad, we're absolutely down to revisit. Want to move quickly and get people trying stuff in the the real world.

@jayphelps jayphelps deleted the replace-epic branch Aug 2, 2016

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