-
Notifications
You must be signed in to change notification settings - Fork 7.6k
1.x: new fromAsync to bridge the callback world with the reactive #4179
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
Conversation
These errors, i.e., Travis running out of memory and killing tests, gets annoying... |
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really awesome tests. Easy to read and understand.
Updated. |
LGTM 👍 ! I'm surprised you couldn't reuse more of the existing infrastructure for the various backpressure modes here and instead need to have a full implementation of the whole queue/drain stuff. |
The codebase spans over several years now and there is no current "best toolset" for building operators. Besides, this inline saves allocation and overhead from applying other operators. |
/cc @stevegury @zsxwing as this is a new operator proposed to the public API |
|
||
t.add(emitter); | ||
t.setProducer(emitter); | ||
asyncEmitter.call(emitter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't invoking this be deferred until request > 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we do that? The subscribe() itself triggers the execution of the body that sets up the push source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I guess it depends on what the implementation of the emitter is. I have a few use cases in my head I'm thinking through. Besides you could use AsyncOnSubscribe
if you need that request coordination.
@JakeWharton how eager are you about this. I'd really love to merge all remaining PRs so 1.1.7 is as complete API-vise as possible. Otherwise, we may have to wait till 1.1.8 and 1.2 is also delayed. |
I would, of course, prefer that it made it. Releases are few and far between here so missing the boat might mean 3 months before it sees the light of day. That said, if no one from Netflix is available to review the API and it's the only thing blocking 1.1.7 then I'm fine with it missing the boat. |
I see low risk as this is a completely new operator. |
Thanks! On Sun, Jul 10, 2016, 2:36 AM David Karnok [email protected] wrote:
|
I wonder should this method be used instead of |
👍 |
This PR adds a new source operator:
fromAsync()
that let's bridge the callback-style world with the reactive world by providing a push surface and offers options to handle backpressure.