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(debounceTime): align value emit behavior as same as RxJS4 #1083

Merged
merged 1 commit into from
Dec 16, 2015

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Dec 16, 2015

due to current null check, debounceTime does not allow value
to be undefined. This change updates behavior to align behavior
as same as RxJS4 does.

closes #1081

@kwonoj
Copy link
Member Author

kwonoj commented Dec 16, 2015

I think this change's legit, but will wait to see if any other opinion / suggestions around before checkin.

@mattpodwysocki
Copy link
Collaborator

@kwonoj why are you checking for null/undefined, when you should have a separate flag instead for hasValue just as we did in RxJS v4? After all, null is also a valid value to send along. Take a look at the current v4 implementation: https://github.com/Reactive-Extensions/RxJS/blob/master/src/modular/observable/debounce.js

@kwonoj
Copy link
Member Author

kwonoj commented Dec 16, 2015

Thanks for suggestion, it's quite valid point. Current implementation used null instead of having hasValue to check it has value or not, so as pointed out it won't be able to accept it as value. I'll update PR to reflect behavior correctly.

@staltz
Copy link
Member

staltz commented Dec 16, 2015

Ah, this PR exists... nice.

due to current null check, debounceTime does not allow value
to be undefined. This change updates behavior to align behavior
as same as RxJS4 does.

closes ReactiveX#1081
@kwonoj
Copy link
Member Author

kwonoj commented Dec 16, 2015

PR updated to introduce hasValue instead of current implementation uses null to check if value has been set or not, to avoid some of values are not being accepted correctly.

@kwonoj kwonoj merged commit 5ee11e0 into ReactiveX:master Dec 16, 2015
@kwonoj
Copy link
Member Author

kwonoj commented Dec 16, 2015

Merged with 5ee11e0.

@kwonoj kwonoj deleted the fix-debounceTime branch December 16, 2015 18:15
@lock
Copy link

lock bot commented Jun 7, 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 7, 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.

debounceTime doesn't work with Subject
3 participants