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

feat(bufferTime): add maxBufferSize optional argument #1556

Closed
wants to merge 1 commit into from

Conversation

figueredo
Copy link
Contributor

Add maxBufferSize to bufferTime as a way of implementing bufferTimeOrCount as discussed in #1295.

@benlesh
Copy link
Member

benlesh commented Mar 30, 2016

I think it would be better for our end users to allow this operator signature to be more polymorphic.

The following should work:

bufferTime(bufferTimeSpan);  // arguments.length === 1
bufferTime(bufferTimeSpan, scheduler);  // arguments.length === 2 && isScheduler(arguments[1])
bufferTime(bufferTimeSpan, bufferCreationInterval); // arguments.length === 2 && !isScheduler(arguments[1])
bufferTime(bufferTimeSpan, bufferCreationInterval, scheduler); // arguments.length === 3 && isScheduler(arguments[2])
bufferTime(bufferTimeSpan, bufferCreationInterval, maxBufferSize); // arguments.length === 3 && !isScheduler(arguments[2])
bufferTime(bufferTimeSpan, bufferCreationInterval, maxBufferSize, scheduler); // arguments.length === 4

basically, you're going to have to check the length of the arguments, and whether or not the last argument isScheduler in order to determine what's been passed.

@benlesh
Copy link
Member

benlesh commented Mar 30, 2016

Other than that, it looks like a solid change. Thanks @figueredo

@figueredo
Copy link
Contributor Author

Yeah, makes much sense, I didn't think of implementing it in that way. I'll change that. Thanks for reviewing, @Blesh.

@figueredo
Copy link
Contributor Author

Updated the PR making the signature more polymorphic.

@benlesh
Copy link
Member

benlesh commented Apr 12, 2016

I was thinking on this change over this last week. I wonder if maybe RxJS needs to add the concept of buffers as a type of thing (ala js-csp). I think I'd like to put this change on hold until that's discussed.

@benlesh
Copy link
Member

benlesh commented Apr 26, 2016

@figueredo I'm unblocking this and I think we can go ahead with a merge. Sorry for the delay. Do you think you can rebase for me?

@figueredo
Copy link
Contributor Author

@Blesh I'll do it later today. What about the buffers idea (#1609)?

@benlesh
Copy link
Member

benlesh commented May 2, 2016

Merged with cf45540... thanks @figueredo for your contribution! Good stuff!

@benlesh benlesh closed this May 2, 2016
@figueredo figueredo deleted the buffertimeorcount branch August 18, 2016 01:57
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants