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.

Sign up for GitHub

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

Jump to bottom

finalize docs are incomplete #4578

Closed
2 of 5 tasks
cartant opened this issue Feb 19, 2019 · 14 comments
Closed
2 of 5 tasks

finalize docs are incomplete #4578

cartant opened this issue Feb 19, 2019 · 14 comments
Labels
docs Issues and PRs related to documentation help wanted Issues we wouldn't mind assistance with.

Comments

@cartant
Copy link
Collaborator

cartant commented Feb 19, 2019 •
edited

Documentation Related To Component:

The finalize operator.

Please check those that apply

  • typo
  • documentation doesn't exist
  • documentation needs clarification
  • error(s) in example
  • needs example

Description Of The Issue

The docs state:

Returns an Observable that mirrors the source Observable, but will call a specified function when the source terminates on complete or error.

finalize will also call its callback if the subscriber explicitly unsubscribes. See this test.

The documentation needs an example, too.

Okay. I clearly pasted the wrong link. It's this test.
@cartant cartant added help wanted Issues we wouldn't mind assistance with. docs Issues and PRs related to documentation labels Feb 19, 2019
@dzhavat
Copy link
Contributor

dzhavat commented Feb 19, 2019

@cartant Hilarious :D The this test link points to a tweet of a dog holding two boiled eggs in its mouth 😂

@cartant
Copy link
Collaborator Author

cartant commented Feb 19, 2019 •
edited

@dzhavat I've added the correct link. (And I've left the mistakenly-pasted link for the amusement of others.)

@lukk84
Copy link

lukk84 commented May 10, 2019

Here is another reproduction of this issue
https://stackblitz.com/edit/rxjs-bsv8sg

@avrim-tr
Copy link

HI, can we confirm that indeed finalize calls it's method in case we unsubscribe manually, before observable's execution reached complete / error ?
This is sad that I discovered this issue here :(

@BioPhoton
Copy link
Contributor

@cartant thanks a lot for the link with the dog. It was really nice having this in between all the issue stuff. Haha

Anyway, i will put it on my todo list.

@ClementVidal
Copy link

Same behavior observed and discussed here:
https://stackoverflow.com/questions/57696249/rxjs-switchmap-and-tap-issue

IMHO this should deserve attention as it somehow introduce a new hookable "moment" in the lifetime of an observable which is not described yet .

@PDavid
Copy link
Contributor

PDavid commented Oct 9, 2019

Hi, I'd like to help with this issue.
I have some experience with RxJS with developing Angular applications at my job.
Maybe this is a silly question, but to which branch should the PR of the documentation improvement be targeted? Should it be master or 6.x branch?

@PDavid PDavid mentioned this issue Oct 9, 2019
Closed
@cartant
Copy link
Collaborator Author

cartant commented Oct 9, 2019

@PDavid Please target master. Thanks.

@PDavid
Copy link
Contributor

PDavid commented Oct 10, 2019

@cartant Thanks, then I'll redo my changes and open a new PR to master.

@cartant
Copy link
Collaborator Author

cartant commented Oct 10, 2019 •
edited

@PDavid Note that you can rebase your local branch (use git rebase --onto ...), push to the PR and then change the PR's branch to master - a dropdown is made available to do so when you edit the PR's title.

@PDavid PDavid mentioned this issue Oct 10, 2019
Merged
@PDavid
Copy link
Contributor

PDavid commented Oct 11, 2019

@cartant Thanks for the rebase --onto hint. ;-) I rebased my branch onto master but needed to open a new PR because I already closed the original PR.
How can we move forward with this?
Who can have a look on the PR and provide feedback?

@cartant
Copy link
Collaborator Author

cartant commented Oct 11, 2019

It's in the queue. I can assure you that your work is appreciated, but we are all busy people. I will try to make some time on the weekend to go through the PR backlog.

@PDavid
Copy link
Contributor

PDavid commented Oct 11, 2019

Sure, thank you! Just wanted to make sure. :)

niklas-wortmann pushed a commit that referenced this issue Nov 30, 2019
@PDavid @niklas-wortmann
docs(finalize): examples, improvement #4578 (#5067)
abbc400 
* docs(angular): migrate app from angular 6 to 8(#4687) (#4688)

* docs(operator): add missing marble diagramms

* Revert "docs(operator): add marbel diagramm to distinctUntilChanged"

This reverts commit 6bf3aae.

* Revert "docs(operator): add fitting description and diagramm to distinctUntilKeyChanged"

This reverts commit 98c23b5.

* Revert "docs(operator): add missing marble diagramms"

This reverts commit b61acad.

* docs(angular): update angular/ codelyzer/ typescript version

* docs(angular): fix minor errors after migration

* chore(eslint): update version

* docs(app): upgrade to angular 8

* docs(migration): fix wrong polyfill imports

* docs(finalize): improved #4578

- Added the fact that finalize will also call its callback if the subscriber explicitly unsubscribes.
- Added example for finalize basic usage and when  subscriber explicitly unsubscribes.

* docs(finalize): implemented requested changes #4578

- Removed unneeded comment (with the numbers).
- Changed example console output to be on separate lines.

* docs(finalize): implemented requested changes #4578

Do not pass ``noop`` to error callback because it will swallow the error and this could be a bad example for newcomers.

* docs(finalize): Revert accidentally pushed commit

"docs(angular): migrate app from angular 6 to 8(#4687) (#4688)"

This reverts commit c440b91
@niklas-wortmann
Copy link
Member

closed by #5065

@lock lock bot locked as resolved and limited conversation to collaborators Jan 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
docs Issues and PRs related to documentation help wanted Issues we wouldn't mind assistance with.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@dzhavat @PDavid @ClementVidal @cartant @niklas-wortmann @BioPhoton @avrim-tr @lukk84