Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[Docs]: add best practice guide #5185

Closed
niklas-wortmann opened this issue Dec 18, 2019 · 17 comments
Closed

[Docs]: add best practice guide #5185

niklas-wortmann opened this issue Dec 18, 2019 · 17 comments
Labels
docs Issues and PRs related to documentation

Comments

@niklas-wortmann
Copy link
Member

In the last core meeting, we came up, that we want to have an official recommendation within the official docs. It should include:

  • Don't sub-class
  • handling subscribe + unsubscribe
  • mention linting rules
  • ...

I'm very open to any kind of suggestion, so please let me know about anything you could think of.

@niklas-wortmann niklas-wortmann added type: discussion docs Issues and PRs related to documentation labels Dec 18, 2019
@thekiba
Copy link

thekiba commented Dec 19, 2019

@jwo719 is wonderful!

I suggest adding the article https://medium.com/p/92502d5639d0 to explain handling subscribe-unsubscribe

@niklas-wortmann
Copy link
Member Author

that's a pretty extensive blog. Thanks for sharing it. It's a little bit too Angular-heavy but there are definitely some points, we should cover in an official best practices recommendation

@BioPhoton
Copy link
Contributor

The section should clarify what .subscribe does. It ends composability.

Subscribe starts a process and/or is here to perform a side effect.

If people want to compose the values further they should not subscribe.

Even if it a very essential basic practice shows this is not clear.

@BioPhoton
Copy link
Contributor

Error handling should also be implemented

@thekiba
Copy link

thekiba commented Dec 20, 2019

Build own operators (i know that rxjs docs already have it but that could be a good point for best practice guide)

The general rule is for takeUntil to be placed last (Avoiding takeUntil Leaks)

@stefanolepera
Copy link

Cover testing as well please, the marble tests can be tricky sometimes. I'd like to see more examples of more complex scenarios.

@dzhavat
Copy link
Contributor

dzhavat commented Dec 20, 2019

I think @cartant 's rxjs-tslint-rules should be mentioned as well.

@lamisChebbi
Copy link

Hi @jwo719
I think it should include also performance tips and common mistakes that can lead to memory leaks.
Thanks

@Armenvardanyan95
Copy link
Contributor

Armenvardanyan95 commented Mar 13, 2021

As far as I know there is no official best practices page in the docs, and I would love to create one, using both my experience with the library (I've written several articles on both good approaches and incorporating RxJS in Angular, see: this, this and this one) and the advice shared by contributors in this thread. Is there anything that could block me? What is a good starting point? Any insight on how that page will be located and what it could look like, also, how much creative freedom can I have if we choose to go forward with it?

@lamisChebbi
Copy link

@Armenvardanyan95 I can contribute on this also

@Armenvardanyan95
Copy link
Contributor

@lamisChebbi thank you. I have created a markdown file where I have written some introductory stuff. I can create a separate repo with it, until we understand how we proceed with that doc inside RxJS docs

@niklas-wortmann
Copy link
Member Author

hi @Armenvardanyan95 & @lamisChebbi please feel free to make suggestions for such a page. It would basically require a seperate md document in the docs_app project and an adjustment in the navigation.json file to make your changes reflect in the navigation menu.
I would highly appreciate suggestions from your side and iterate over a pr together with you, if that would work for you?
Is there anything else I could help you with to get you started with it?

@Armenvardanyan95
Copy link
Contributor

Armenvardanyan95 commented Mar 18, 2021

@niklas-wortmann thanks a lot. I have started working on an md file, I will continue working on an initial version of it for a day or two and then submit a pr, then we can probably discuss it more concretely. As of now I don't have specific questions, there is lots of thoughts in this very thread I can use as guidance, we will see then how it goes

@Armenvardanyan95
Copy link
Contributor

@niklas-wortmann Although upon re-reading this thread I have q question. You mentioned in the very first comment "don't subclass", do you mean that we should not subclass Observable classes and use customized versions? Why is that so? I have not been aware this is an antipattern

@niklas-wortmann
Copy link
Member Author

yeah that's exactly what I meant. We came around this issue a couple of times, here's one reference I found quickly:
#5431

The problem is basically, that it is quite easy to shoot yourself in the foot and it might break when we do changes to those classes as there are very tightly coupled this way

@Armenvardanyan95
Copy link
Contributor

Thank you. I will add that to the doc

Armenvardanyan95 added a commit to Armenvardanyan95/rxjs-1 that referenced this issue Mar 19, 2021
@Armenvardanyan95
Copy link
Contributor

@lamisChebbi @niklas-wortmann I submitted an initial PR, would be happy to continue discussion there and hear opinions on how it can be improved

#6157

@benlesh benlesh closed this as completed May 4, 2021
@ReactiveX ReactiveX locked and limited conversation to collaborators May 4, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
docs Issues and PRs related to documentation
Projects
None yet
Development

No branches or pull requests

8 participants