docs: add a best practices document #6157
Conversation
|
Important questions to be addressed:
WIll happily listen to everyone's thoughts |
There was a problem hiding this comment.
Hi sorry I wasn't able to review this earlier. First of all thanks for taking the time for this contribution, I do think that this is a gap in the docs we have to fill. From my point of view your suggestions have a couple personal opinions, that I don't necessarily share and I am not sure how we should incorporate those properly in the docs. Maybe other core team members have an opinion as well, @cartant, @benlesh, @kwonoj, @kolodny
| @@ -0,0 +1,100 @@ | |||
| ### RxJS best practices | |||
There was a problem hiding this comment.
the headline structure is kinda weird, why is this a h3? Shouldn't this be an h1?
|
|
||
| RxJS is very large and contains lots of functions, operators and other stuff which makes it a complex ecosystem, but for lots of applications it can be as simple as knowing what `Observables` are and what can be done with them with some `operators`. But for larger codebases it is important to have code written in a way that allows for reusability, readability and composability. This guide may help overcome common ba practices and adopt a reactive mindset necessary for righting concise RxJS code. | ||
|
|
||
| ## .subscribe ends composability |
There was a problem hiding this comment.
I personally don't fully agree with this and would not put this in a best practice guide, but maybe the other core team members have a different opinion
| When we call `.subscribe` on any `Observable` what we do is get rid of said `Observable` and then only focus on the values that it produces. When we call `.subscribe` we can no longer continue composing operators (`.subscribe` returns a `Subscription`, on which we can no longer call `.pipe` and use RxJS operators), and composability is **the** greatest feature of RxJS. Obviously, without subscribing to most `Observables`, we won't have a working application. In practice, frameworks like Angular offer solutions that allow to use `Observable` values directly without explicitly subscribing to them, a great example is the `async` pipe. | ||
| So while subscribing is obviously not directly discouraged, we should be generally very careful when doing it, as to understand that the stream in question is no longer to be modified and can only be used as a source of values, nothing more. | ||
|
|
||
| ## Using imperative logic in .subscribe instead of operators |
There was a problem hiding this comment.
I also don't necessarily agree with this. There are valid use cases and there is nothing wrong with this approach. I actually think that people more likely harm readability by leveraging all weird operators
|
|
||
| Another problem that developers face when using `.subscribe` is performing complex operations on values from a source `Observable`. Especially bad scenarios involve using conditional operators and loops inside `.subscribe` callbacks, instead of composing `Observables` using pipes. Here are some common mistakes: | ||
|
|
||
| ### Using an if statement when subscribing |
There was a problem hiding this comment.
I would not consider this a best practice. It's more of a preference from your side.
| }); | ||
|
|
||
|
|
||
| ### In general, heavy logic when subscribing is discouraged |
There was a problem hiding this comment.
Again, I don't really agree with this. There is nothing wrong with subscribing to an Observable it is actually required and this might be more confusing for newcomers.
| takeUntil(notifier$), | ||
| ).subscribe(console.log) | ||
|
|
||
| In this example just looking at the `Observable` is enough to know exactly when it will terminate: when the `notifier$` `Observable` emits. Also, this is the widely accepted approach in the Angular community. |
There was a problem hiding this comment.
The docs are and should be framework agnostic so I would prefer not to refer to angular here
|
|
||
| ## Avoiding memory leaks | ||
|
|
||
| The way RxJS operates creates a way for memory leaks to appear in applications that use it. Usually it happens when a consumer subscribes to an `Observable` and the `Observable` itself never terminates naturally (for example, an `Observable` created by `interval`, which emits values in equal periods of time), and no one unsubscribes from it or terminates it manually. The simplest way to do that is directly unsubscribe when the subscription is no longer necessary: |
There was a problem hiding this comment.
I'd like a little bit more explanations here, how this problem can happen and what the problem would cause. This might be very hard to understand for people new to rxjs
|
|
||
| ### Be careful to avoid takeUntil leaks | ||
|
|
||
| When `takeUntil` is not used correctly, it can lead to memory leaks too. As a rule of thumb, always place `takeUntil` as the last operator in the `pipe`. |
There was a problem hiding this comment.
can you elaborate on this a little bit to make it easier to understand for newcomers
|
|
||
| ## Don't subclass | ||
|
|
||
| Sometimes, it can be tempting to build our own version of an `Observable` using existing classes. In particular, some developers might extend the existing `Observable` class to add some functionality and then use the "better" version throughout the app. But problems can arise in the future, for example, if the base classes are updated with the new versions of the RxJS library, as those classes are tightly coupled. Also, implementing a bae class that can work requires the developer to implement the `lift` method, which in turn makes the consumer application aware of the internal implementation details of the library. This [thread](https://github.com/ReactiveX/rxjs/issues/5431) contains a detailed discussion of the matter. |
I don't think this could be merged without discussion. I don't agree with everything in this PR and, FWIW, I'm reluctant to declare a list of official best practices. |
|
Regarding the don't subclass section. Can you provide an alternative to this kind of usecase? Suppose you had a domain class like Would that mean some sort of One issue I'm thinking is that the |
This is an initial version of a document describing best practices and antipatterns to avoid when using RxJS. This is a more finalized version of my draft, I tried to bring together important points mentioned by everyone in the related issue thread (#5185). Will be glad to hear how this can be improved