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(createEpicMiddleware): Don't share a scheduler queue with anyone else's RxJS code #625

merged 1 commit into from Mar 26, 2019


Copy link

@jayphelps jayphelps commented Mar 21, 2019

fixes #624

Since we use the same queueScheduler that other people's app code might also use, we would get different behavior if the rootEpic is passed to inside an already running queueScheduler task. Before this PR we incorrectly relief on the fact that almost always this wouldn't happen, but it can. If it does, redux-observable won't subscribe to the rootEpic soon enough--it'll be deferred until the current queue task returns, potentially missing actions.

RxJS doesn't currently publicly export QueueScheduler constructor nor QueueAction, so we reach in using arguably private APIs.

@benlesh this is doing naughty things, reaching into (currently) private APIs. If you have a moment to give this a quick once over, lmk if you have alternative suggestions. If no time, no worries.

{ type: 'ACTION_4' }

Copy link
Member Author

@jayphelps jayphelps Mar 21, 2019

Choose a reason for hiding this comment

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

This isn't really "async" in the sense of needing this done() but I prefer to have it regardless in case of later refactors later that do in fact introduce async that might cause the test to pass without actually making any assertions.

@jayphelps jayphelps merged commit e5bae19 into master Mar 26, 2019
2 checks passed
Copy link
Member Author

jayphelps commented Mar 26, 2019

Landed in v1.1.0

@MaximeBernard MaximeBernard deleted the fix-624 branch May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

3 participants