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(windowTime): clean up closed window with timeSpanOnly #2240

Closed
wants to merge 1 commit into from

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Jan 2, 2017

Description:
This PR adds cleanup to windowTime operator to clear up closed instance of window when invoked via timespan only.

Related issue (if exists):

@coveralls
Copy link

coveralls commented Jan 2, 2017

Coverage Status

Coverage decreased (-0.0003%) to 97.689% when pulling 181c5ec on kwonoj:fix-windowtime-cleanup into 6922b16 on ReactiveX:master.

@kwonoj
Copy link
Member Author

kwonoj commented Jan 2, 2017

Not sure how to test this cases? :/

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

Break the typings changes and formatting stuff into a separate commit form the important fix, so it's easier to discern the effect of the changes.

const { subscriber, windowTimeSpan, window } = state;
if (window) {
window.complete();
subscriber.closeWindow(window);
Copy link
Member

Choose a reason for hiding this comment

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

@kwonoj should've probably saved all of the typing refactors and formatting stuff for a different commit. It's hard for me to see what the "meat" of this PR is.. I'm assuming it's mostly this line here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, correct. previously timspan'ed window closed, but not teared down as same as default window behavior. Now both uses same teardown path. Sorry for creating commit include mixed changes, should I split up and update PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we might want to. This is a commit to merge in a fix to production code... so we might want to make sure the changes are isolated this time.

Copy link
Member Author

Choose a reason for hiding this comment

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

will do. I'll rather create as separate PR to have better scoped clarity.

@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