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

observeOnce did not unsubscribe correctly #1872

Closed
compulim opened this issue Apr 4, 2019 · 0 comments · Fixed by #2140
Closed

observeOnce did not unsubscribe correctly #1872

compulim opened this issue Apr 4, 2019 · 0 comments · Fixed by #2140
Assignees
Labels
bug Indicates an unexpected problem or an unintended behavior. front-burner p2 Nice to have

Comments

@compulim
Copy link
Contributor

compulim commented Apr 4, 2019

Background

https://github.com/Microsoft/BotFramework-WebChat/blob/master/packages/core/src/sagas/effects/observeOnce.js#L11

In this line of code, we didn't assign the result of observable.subscribe() to subscription. Thus, subscription is always empty and thus, we never call unsubscribe().

Things to-do

  1. Modify https://github.com/Microsoft/BotFramework-WebChat/blob/master/packages/core/src/sagas/effects/observeOnce.js#L11
    • Assign the result of observable.subscribe() to subscription variable
  2. Verify unsubscribe() is being called correctly
  3. Verify all callers of observeOnce will continues to work
@compulim compulim added the bug Indicates an unexpected problem or an unintended behavior. label Apr 4, 2019
@compulim compulim added backlog Out of scope for the current iteration but it will be evaluated in a future release. p2 Nice to have T-Shirt: XS community-help-wanted This is a good issue for a contributor to take on and submit a solution and removed Triage-E labels Apr 4, 2019
@cwhitten cwhitten added the 4.5 label May 13, 2019
@corinagum corinagum added Approved front-burner and removed backlog Out of scope for the current iteration but it will be evaluated in a future release. community-help-wanted This is a good issue for a contributor to take on and submit a solution labels May 13, 2019
@compulim compulim assigned compulim and unassigned corinagum Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or an unintended behavior. front-burner p2 Nice to have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants