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(createEpicMiddleware): schedule emitted actions and epic subscription on the queueScheduler #493

Merged
merged 1 commit into from May 23, 2018

Conversation

Projects
None yet
4 participants
@jayphelps
Member

jayphelps commented May 15, 2018

BREAKING CHANGE:

You must now provide your rootEpic to epicMiddleware.run(rootEpic) instead of passing it to createEpicMiddleware. This fixes issues with redux v4 where it's no longer allowed to dispatch actions while middleware is still being setup. Redux v4 support will be coming in a follow up PR.

This also removes epicMiddleware.replaceEpic

BREAKING CHANGE:

All actions dispatched by both your epics and incoming from the outside world are scheduled on a queue, as is the actual work of subscribing to your root epic.

In 1.0.0 we now subscribe to your root Epic, and dispatch actions emitted by it, using the queueScheduler from RxJS. This is a bit hard to explain (and understand) but as the name suggests, a queue is used. If the queue is empty, the action is emitted as usual, but if that action synchronously causes other actions to be emitted they will be queued up until the call stack of the first action returns.

In a large majority of cases this will have no perceivable impact, but it may affect the order of any complex epic-to-epic communication you have.

One benefit is that actions which are emitted by an epic on start up are not missed by epics which come after it. e.g. With combineEpics(epic1, epic2) previously if epic1 emitted on startup, epic2 would not receive that action because it had not yet been set up. It also changes the potential order of complex epic-to-epic communication in a way that most may find more intuitive.

Take this example:

https://jsbin.com/buvibaf/1/edit

const epic1 = action$ =>
  action$.pipe(
    ofType('FIRST'),
    mergeMap(() =>
      of({ type: 'SECOND' }, { type: 'THIRD' })
    )
  );

const epic2 = action$ =>
  action$.pipe(
    ofType('SECOND'),
    map(() => ({ type: 'FOURTH' })),
    startWith({ type: 'FIRST' })
  );

// notice that epic2 comes *after* epic1
const rootEpic = combineEpics(epic1, epic2);

In older version of redux-observable, your reducers would have been missing the FOURTH:

FIRST
SECOND
THIRD

However in 1.0.0 it now would see it as the last one:

FIRST
SECOND
THIRD
FOURTH

In that example, the SECOND action is now seen by epic2 because it is queued on the same schedule as subscribing (setting up) the Epics themselves is. Since the middleware will try to subscribe to the Epics first, it now always will finish doing so before any action is emitted--so epic2 doesn't miss any actions.

Another way of looking at it is that when an individual Epic is synchronously emitting actions, they will always be emitted in the sequence provided, without any other Epics being able to sneak another action in-between. When we did of({ type: 'SECOND' }, { type: 'THIRD' }), we now know for sure that THIRD will immediately follow SECOND; in older versions of redux-observable this wasn't guaranteed as another Epic could have been listening for SECOND and emitted some other action before THIRD, because they shared the same call-stack.

Because this is dealing with very complex recursion, call stacks, and sequences, this may be tough to fully wrap your head around. We hope that what actually happens in practice is itself more intuitive, even if truly understanding how things are queued is now.

I'm very open if someone has a better way of explaining this. FWIW redux-saga and ngrx both do queue scheduling as well, though the behavior is different between us three in certain edge cases.

@jayphelps jayphelps requested a review from evertbouw May 15, 2018

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 15, 2018

Member

I'm a bit conflicted about the queue scheduling behavior. I personally think people will find it more intuitive in their average code, but it is true that if you have a very good understanding and mental picture of call stacks, this behavior would in fact end up being unintuitive. While most of us would hope we have that level of understanding, in practice I've seen not only myself but many others struggle with these epic-to-epic interactions.

I'm curious what others think. Should we keep things as-is, go this route, or something else?

Member

jayphelps commented May 15, 2018

I'm a bit conflicted about the queue scheduling behavior. I personally think people will find it more intuitive in their average code, but it is true that if you have a very good understanding and mental picture of call stacks, this behavior would in fact end up being unintuitive. While most of us would hope we have that level of understanding, in practice I've seen not only myself but many others struggle with these epic-to-epic interactions.

I'm curious what others think. Should we keep things as-is, go this route, or something else?

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 15, 2018

Member

Here's a jsbin that lets you play around and compare the behavior of the new/old:

https://jsbin.com/buvibaf/1/edit

Change useQueueScheduling = false in the HTML tab to switch. Play around with various complex and recursive epic-to-epic communication.

Member

jayphelps commented May 15, 2018

Here's a jsbin that lets you play around and compare the behavior of the new/old:

https://jsbin.com/buvibaf/1/edit

Change useQueueScheduling = false in the HTML tab to switch. Play around with various complex and recursive epic-to-epic communication.

@evertbouw

I really don't know what impact the queueScheduler will have. I'll need to sleep on it and look at it again tomorrow. Are there alternatives you are thinking about?

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 15, 2018

Member

Are there alternatives you are thinking about?

It's currently doing three schedules using the same queueScheduler: one for subscribe, one that observes the output of epics, and one that observes the input of epics. All three are needed to achieve the behavior I think is ideal, but we could potentially do something totally different scheduling wise or just keep the status-quo. I'm hoping someone else can challenge my thought process on this, even just to confirm it is indeed sound or if I'm mistaken.

Member

jayphelps commented May 15, 2018

Are there alternatives you are thinking about?

It's currently doing three schedules using the same queueScheduler: one for subscribe, one that observes the output of epics, and one that observes the input of epics. All three are needed to achieve the behavior I think is ideal, but we could potentially do something totally different scheduling wise or just keep the status-quo. I'm hoping someone else can challenge my thought process on this, even just to confirm it is indeed sound or if I'm mistaken.

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 15, 2018

Member

Examples

Here are some examples to help grok the differences in complex epic-to-epic communication. On one hand, the current behavior is technically "correct" my feeling is that it isn't as intuitive as the behavior would be with the queueScheduler. I feel like it helps make epics feel more like separate processes which interact via message passing, instead of the current situation in which the callstack of one can be interrupted by another epic.

I'm very curious if others agree/disagree.

synchronous emit on startup

Currently the order of your epics provided to combineEpics matters not just by the order in which they'll receive actions (unavoidable as single threaded) but it also means that if an epic emits synchronously on start up, any epic that comes after it won't receive it because they are not yet subscribed to.

const epic1 = action$ =>
  action$.pipe(
    ofType('SOMETHING'),
    startWith({ type: 'FIRST' })
  );

const epic2 = action$ =>
  action$.pipe(
    ofType('FIRST'),
    map(() => ({ type: 'SECOND' }))
  );

const rootEpic = combineEpics(epic1, epic2);

Current

FIRST

(no SECOND action because epic2 isn't listening in time to catch FIRST)

With queue scheduling

FIRST
SECOND

synchronously emitting sequential actions

In the current version if an epic tries to emit more than one action synchronously and sequentially, it's possible that another epic could be listening and emit it's own action before the originating epic has finished dispatching it's own.

const epic1 = action$ =>
  action$.pipe(
    ofType('FIRST'),
    mergeMap(() =>
      of({ type: 'SECOND' }, { type: 'THIRD' })
    )
  );

const epic2 = action$ =>
  action$.pipe(
    ofType('SECOND'),
    map(() => ({ type: 'FOURTH' }))
  );

Current

FIRST
SECOND
FOURTH
THIRD

(FOURTH is before THIRD)

With queue scheduling

FIRST
SECOND
THIRD
FOURTH

This behavior could also been seen in even more complicated scenarios where an epic is listening for the action it also emits itself:

const epic1 = action$ =>
  action$.pipe(
    ofType('FIRST'),
    mergeMap(() =>
      merge(
        action$.pipe(
          ofType('SECOND'),
          map(() => ({ type: 'FIFTH' }))
        ),
        of({ type: 'SECOND' }, { type: 'THIRD' })
      )
    )
  );

const epic2 = action$ =>
  action$.pipe(
    ofType('SECOND'),
    map(() => ({ type: 'FOURTH' }))
  );

Current

FIRST
SECOND
FOURTH
FIFTH
THIRD

(FOURTH and FIFTH is before THIRD)

With queue scheduling

FIRST
SECOND
THIRD
FOURTH
FIFTH

takeUntil patterns may also not subscribe early enough

Some patterns I've seen using takeUntil also currently have the issue where they are unintuitively not yet subscribed when someone thinks they are.

const epic1 = action$ =>
  action$.pipe(
    ofType('FIRST'),
    mergeMap(() =>
      timer(100).pipe(
        map(() => ({ type: 'FOURTH' })),
        takeUntil(action$.pipe(
          ofType('THIRD')
        )),
        startWith({ type: 'SECOND' })
      )
    )
  );

const epic2 = action$ =>
  action$.pipe(
    ofType('SECOND'),
    map(() => ({ type: 'THIRD' }))
  );

Current

FIRST
SECOND
THIRD
FOURTH

(FOURTH is emitted because the takeUntil wasn't yet subscribing to see that THIRD was dispatched by epic2 so it misses it)

With queue scheduling

FIRST
SECOND
THIRD

(FOURTH is not emitted because that Observable was cancelled because epic2 emitted THIRD and it's seen)

Member

jayphelps commented May 15, 2018

Examples

Here are some examples to help grok the differences in complex epic-to-epic communication. On one hand, the current behavior is technically "correct" my feeling is that it isn't as intuitive as the behavior would be with the queueScheduler. I feel like it helps make epics feel more like separate processes which interact via message passing, instead of the current situation in which the callstack of one can be interrupted by another epic.

I'm very curious if others agree/disagree.

synchronous emit on startup

Currently the order of your epics provided to combineEpics matters not just by the order in which they'll receive actions (unavoidable as single threaded) but it also means that if an epic emits synchronously on start up, any epic that comes after it won't receive it because they are not yet subscribed to.

const epic1 = action$ =>
  action$.pipe(
    ofType('SOMETHING'),
    startWith({ type: 'FIRST' })
  );

const epic2 = action$ =>
  action$.pipe(
    ofType('FIRST'),
    map(() => ({ type: 'SECOND' }))
  );

const rootEpic = combineEpics(epic1, epic2);

Current

FIRST

(no SECOND action because epic2 isn't listening in time to catch FIRST)

With queue scheduling

FIRST
SECOND

synchronously emitting sequential actions

In the current version if an epic tries to emit more than one action synchronously and sequentially, it's possible that another epic could be listening and emit it's own action before the originating epic has finished dispatching it's own.

const epic1 = action$ =>
  action$.pipe(
    ofType('FIRST'),
    mergeMap(() =>
      of({ type: 'SECOND' }, { type: 'THIRD' })
    )
  );

const epic2 = action$ =>
  action$.pipe(
    ofType('SECOND'),
    map(() => ({ type: 'FOURTH' }))
  );

Current

FIRST
SECOND
FOURTH
THIRD

(FOURTH is before THIRD)

With queue scheduling

FIRST
SECOND
THIRD
FOURTH

This behavior could also been seen in even more complicated scenarios where an epic is listening for the action it also emits itself:

const epic1 = action$ =>
  action$.pipe(
    ofType('FIRST'),
    mergeMap(() =>
      merge(
        action$.pipe(
          ofType('SECOND'),
          map(() => ({ type: 'FIFTH' }))
        ),
        of({ type: 'SECOND' }, { type: 'THIRD' })
      )
    )
  );

const epic2 = action$ =>
  action$.pipe(
    ofType('SECOND'),
    map(() => ({ type: 'FOURTH' }))
  );

Current

FIRST
SECOND
FOURTH
FIFTH
THIRD

(FOURTH and FIFTH is before THIRD)

With queue scheduling

FIRST
SECOND
THIRD
FOURTH
FIFTH

takeUntil patterns may also not subscribe early enough

Some patterns I've seen using takeUntil also currently have the issue where they are unintuitively not yet subscribed when someone thinks they are.

const epic1 = action$ =>
  action$.pipe(
    ofType('FIRST'),
    mergeMap(() =>
      timer(100).pipe(
        map(() => ({ type: 'FOURTH' })),
        takeUntil(action$.pipe(
          ofType('THIRD')
        )),
        startWith({ type: 'SECOND' })
      )
    )
  );

const epic2 = action$ =>
  action$.pipe(
    ofType('SECOND'),
    map(() => ({ type: 'THIRD' }))
  );

Current

FIRST
SECOND
THIRD
FOURTH

(FOURTH is emitted because the takeUntil wasn't yet subscribing to see that THIRD was dispatched by epic2 so it misses it)

With queue scheduling

FIRST
SECOND
THIRD

(FOURTH is not emitted because that Observable was cancelled because epic2 emitted THIRD and it's seen)

@evertbouw

I'm only seeing benefits here but you do have doubts about the queueSchedule. Are there actual downsides to this? Or is it just because you are comfortable working with a stack and a queue requires a different mental model?

If we need user feedback we should make a release with these changes.

@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 16, 2018

Member

Or is it just because you are comfortable working with a stack and a queue requires a different mental model?

I definitely still get bitten by deep epic-to-epic callstack complexities, but I've run into it enough times helping others that I often can spot the pattern quickly. My thought was that this queue usage would make things more intuitive for the average user however there's a decent risk that this will actually trip someone up who is expecting the previous behavior. I'm also afraid that this is a higher risk because I haven't figured out a great way to explain it without just showing numerous before/after examples, which makes me second guess this.

I think I'll keep this open for a few days or a week for comments and if there are no objections go ahead and merge and release it as another alpha to see if we broke people in ways that aren't for the better.

Member

jayphelps commented May 16, 2018

Or is it just because you are comfortable working with a stack and a queue requires a different mental model?

I definitely still get bitten by deep epic-to-epic callstack complexities, but I've run into it enough times helping others that I often can spot the pattern quickly. My thought was that this queue usage would make things more intuitive for the average user however there's a decent risk that this will actually trip someone up who is expecting the previous behavior. I'm also afraid that this is a higher risk because I haven't figured out a great way to explain it without just showing numerous before/after examples, which makes me second guess this.

I think I'll keep this open for a few days or a week for comments and if there are no objections go ahead and merge and release it as another alpha to see if we broke people in ways that aren't for the better.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist May 17, 2018

After thinking about it it seems to me like a good direction.

I share ur concern that this might not be intuitive for advanced users - that's why it's important to mention this ASAP in the docs and explain well that certain assumptions might be wrong because of the queueing behaviour (I would hide examples in separate section that could be linked in the introduction, to not overwhelm beginners).

I believe this breaks less assumptions than previous behaviour, even for advanced users it should make life easier because I believe that most people just don't think about exact ordering of things when coding with messaging patterns - it's ofc good to know how things work when your code stops to work, but thinking about it is somewhat an overhead and a burden put on a developer and this queueing tries to alleviate that.

I'm also surprised that implementing this was so easy, RxJS 👏

Andarist commented May 17, 2018

After thinking about it it seems to me like a good direction.

I share ur concern that this might not be intuitive for advanced users - that's why it's important to mention this ASAP in the docs and explain well that certain assumptions might be wrong because of the queueing behaviour (I would hide examples in separate section that could be linked in the introduction, to not overwhelm beginners).

I believe this breaks less assumptions than previous behaviour, even for advanced users it should make life easier because I believe that most people just don't think about exact ordering of things when coding with messaging patterns - it's ofc good to know how things work when your code stops to work, but thinking about it is somewhat an overhead and a burden put on a developer and this queueing tries to alleviate that.

I'm also surprised that implementing this was so easy, RxJS 👏

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk May 18, 2018

Member

Based on everything written above, I'm currently in favor of using the queue scheduler.

Member

rgbkrk commented May 18, 2018

Based on everything written above, I'm currently in favor of using the queue scheduler.

feat(createEpicMiddleware): schedule emitted actions and epic subscri…
…ption on the queueScheduler, so that epic order matters less

BREAKING CHANGE:

You must now provide your rootEpic to `epicMiddleware.run(rootEpic)` instead of passing it to `createEpicMiddleware`. This fixes issues with redux v4 where it's no longer allowed to dispatch actions while middleware is still being setup. See https://redux-observable.js.org/MIGRATION.html

BREAKING CHANGE:

`epicMiddleware.replaceEpic` has been removed. A the equivilant behavior can be accomplished by dispatching your own `END` action that your rootEpic is listening for with a `takeUntil`, then providing the next rootEpic to `epicMiddleware.run(nextRootEpic)`. See https://redux-observable.js.org/MIGRATION.html

BREAKING CHANGE:

Actions your epics emit are now scheduled using the queueScheduler. This is a bit hard to explain (and understand) but as the name suggests, a queue is used. If the queue is empty, the action is emitted as usual, but if that action causes other actions to be emitted they will be queued up until the call stack of the first action returns.

In a large majority of cases this will have no perceivable impact, but it may affect the order of any complex epic-to-epic communication you have.

The benefit is that actions which are emitted by an epic on start up are not missed by epics which come after it. e.g. With `combineEpics(epic1, epic2)` previously if epic1 emitted on startup, epic2 would not receive that action because it had not yet been set up. See https://redux-observable.js.org/MIGRATION.html
@jayphelps

This comment has been minimized.

Show comment
Hide comment
@jayphelps

jayphelps May 23, 2018

Member

Thanks to everyone for discussions and reviews.

@Andarist thanks again for discussing with me in DMs and giving insight into what's worked for redux-saga. <3

Member

jayphelps commented May 23, 2018

Thanks to everyone for discussions and reviews.

@Andarist thanks again for discussing with me in DMs and giving insight into what's worked for redux-saga. <3

@jayphelps jayphelps merged commit 6356816 into master May 23, 2018

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

@rgbkrk rgbkrk deleted the queue branch May 24, 2018

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