Allow using startWith/from/of inside epics. #254

franekp · 2017-05-30T22:47:21Z

Closes #253

[+] I have read the Contributor Guide
[+] My commit messages are in conventional-changelog-standard format.

jayphelps · 2017-05-31T02:08:55Z

src/createEpicMiddleware.js

@@ -45,6 +48,7 @@ export function createEpicMiddleware(epic, options = defaultOptions) {
          return output$;
        })
        ::switchMap(output$ => options.adapter.output(output$))
+        ::mergeMap(action => of(action, asap))


hmm I'm pretty reluctant to schedule every action asap. Seems like it could create hard to understand behavior for people, as well as more complex stack traces--separately, I think ::observeOn(asap) would be the more idiomatic way of achieving asap scheduling, though they both would have the same net result seemingly. Don't worry about changing it just yet, need to figure out out if this is the best solution in general.

I tried using ::observeOn(asap) first, but it broke the it('should throw if you provide a root epic that doesn\'t return anything)' since exception was thrown from the asap scheduler and not from createStore(..., applyMiddleware(...)) call.

oh right cause observeOn will schedule everything whereas mergeMap+of(a, asap) will just schedule next, not error/complete

jayphelps · 2017-05-31T02:15:20Z

Will need to think about this one some more, but my gut thinks the solution is to somehow wait until after the middleware is finished setting up before subscribing to the root epic. That's tricky because of the way the redux middleware API is designed as well as we want to be careful not to miss any actions dispatched synchronously outside of redux-observable (e.g. in server rendering cases this is common)

I'm pretty reluctant to schedule all actions on asap scheduler cause that feels like it might cause other problems and make the normal flow harder to reason for people--it also wouldn't be as performant but I'm not as worried about that.

btw this is definitely a bug. Your use case definitely should be supported, so thank you for bringing it up! I thought I had tested that, but seems I did not (didn't test the recursive nature)

BerkeleyTrue · 2017-06-01T14:22:40Z

test/createEpicMiddleware-spec.js

+  for (const _ of new Array(10)) {  // eslint-disable-line no-unused-vars
+    await (new Promise(setTimeout));
+  }
+};


Why not use Observables to declaratively add these delays?

Sure, I will add change it.

franekp · 2017-06-01T22:03:05Z

the solution is to somehow wait until after the middleware is finished setting up before subscribing to the root epic.

Hmm, it turns out that there are 2 problems here:

Actions dispatched during middleware initialization do not reach middleware
Dispatching in a middleware before applyMiddleware completes reduxjs/redux#1240 - seems to not going to be fixed on redux side
"race condition" between epic$.next(...) and synchronous actions emitted by epic:
my guess is that somehow these actions are emitted before epic$.next(...) wires
action$ to the input of new epic.

Waiting until after middleware is initialized solves (1) but not (2).

BerkeleyTrue · 2017-06-01T22:10:47Z

@franekp I'd wait to make any changes to this PR.

The most likely scenario is we move to a store enhance in place of a middleware. This will give us the flexibility to manage some edge cases that have popped up as well as add some power features not available to use using middleware API.

See: #256

… now processed by epics. Closes redux-observable#253

franekp · 2017-06-01T22:23:35Z

I'd wait to make any changes

Hmm, too late.

@jayphelps Fixed without changing schedulers now, please review.

mpiroc · 2017-08-26T17:12:51Z

Any update on this? I think it may be the root cause of a tricky bug I'm struggling with in my app.

jayphelps · 2017-08-28T21:15:19Z

@mpiroc Should be fairly easy to verify--check if you are emitting an action immediately on app start via startWith or concat, and another epic is listening for this action but not receiving it but your reducers are. This is a fairly odd pattern IMO, but I don't doubt there are some rare use cases for it.

We're switching from a middleware to a store enhancer in #256. This is progressing, but admittedly slow because this is all volunteer work.

mpiroc · 2017-08-28T21:27:33Z

Thanks @jayphelps, my bug turned out to be unrelated to this issue.

jayphelps · 2017-09-29T22:32:55Z

@franekp I feel pretty bad that that is a pretty impressive PR, but has sat unmerged for so long. I'm sorry! It's still a use case I think we should support, I just don't have confidence yet this approach is the way to achieve it. I'm going to try to figure out #256 in the coming weeks, then we can circle back on this.

franekp · 2017-10-02T08:55:05Z

No problem, I understand. I am glad you want to support this use case.

…s now supported. fixes #253 closes #254, #259, #256 BREAKING CHANGE: To support synchronously emitting actions when an Epic starts up, along with other fairly complex recursive action use cases, your root Epic is now subscribed to using the Queue Scheduler, and then any actions you emit are also scheduled using the Queue Scheduler. It's possible that some of your Epics could be relying on the fact that an action was historically emitted without Queue scheduling, most likely unknowingly. If none of your Epics rely on the actions emitted by another Epic or itself, you should see no changes. As this is a complex topic to elaborate on, and the Queue Scheduler is not well known, you'll want to learn more here: https://redux-observable.js.org/MIGRATION.html

jayphelps · 2019-11-15T00:08:59Z

I believe this was fixed last year. Definitely lmk if not! 62a8c73

franekp changed the title ~~fix(createEpicMiddleware): Actions emitted with startWith/of/from are…~~ Allow using startWith/from/of inside epics. May 30, 2017

jayphelps reviewed May 31, 2017

View reviewed changes

jayphelps mentioned this pull request May 31, 2017

Actions emited with Observable.{startWith,of,from} are swallowed silently. #253

Open

BerkeleyTrue reviewed Jun 1, 2017

View reviewed changes

fix(createEpicMiddleware): Actions emitted with startWith/of/from are…

92332c5

… now processed by epics. Closes redux-observable#253

franekp force-pushed the fix-swallowed-actions branch from eb74c42 to 92332c5 Compare June 1, 2017 22:13

jayphelps mentioned this pull request Jan 24, 2018

feat(state$): The second argument of an Epic is now a stream of state$, not a store #410

Merged

2 tasks

wldcordeiro mentioned this pull request May 24, 2018

[Feature Request] Add "mapToAction" utility operator #457

Open

jayphelps closed this Nov 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using startWith/from/of inside epics. #254

Allow using startWith/from/of inside epics. #254

franekp commented May 30, 2017 •

edited

jayphelps May 31, 2017 •

edited

franekp Jun 1, 2017

jayphelps Jun 1, 2017

jayphelps commented May 31, 2017 •

edited

BerkeleyTrue Jun 1, 2017

franekp Jun 1, 2017

franekp commented Jun 1, 2017 •

edited

BerkeleyTrue commented Jun 1, 2017

franekp commented Jun 1, 2017 •

edited

mpiroc commented Aug 26, 2017

jayphelps commented Aug 28, 2017

mpiroc commented Aug 28, 2017

jayphelps commented Sep 29, 2017

franekp commented Oct 2, 2017

jayphelps commented Nov 15, 2019

Allow using startWith/from/of inside epics. #254

Allow using startWith/from/of inside epics. #254

Conversation

franekp commented May 30, 2017 • edited

jayphelps May 31, 2017 • edited

Choose a reason for hiding this comment

franekp Jun 1, 2017

Choose a reason for hiding this comment

jayphelps Jun 1, 2017

Choose a reason for hiding this comment

jayphelps commented May 31, 2017 • edited

BerkeleyTrue Jun 1, 2017

Choose a reason for hiding this comment

franekp Jun 1, 2017

Choose a reason for hiding this comment

franekp commented Jun 1, 2017 • edited

BerkeleyTrue commented Jun 1, 2017

franekp commented Jun 1, 2017 • edited

mpiroc commented Aug 26, 2017

jayphelps commented Aug 28, 2017

mpiroc commented Aug 28, 2017

jayphelps commented Sep 29, 2017

franekp commented Oct 2, 2017

jayphelps commented Nov 15, 2019

franekp commented May 30, 2017 •

edited

jayphelps May 31, 2017 •

edited

jayphelps commented May 31, 2017 •

edited

franekp commented Jun 1, 2017 •

edited

franekp commented Jun 1, 2017 •

edited