feat(Subscription): add no longer returns unnecessary Subscription …
#5656
Conversation
|
This is an important step toward fixing some critical internal pieces of our library. |
benlesh
force-pushed
the
feat/Subscription-add-returns-void
branch
2 times, most recently
from
54eeda7
to
e155442
Compare
| @@ -170,14 +169,14 @@ export class Subscription implements SubscriptionLike { | |||
|
|
|||
| // Add `this` as parent of `subscription` if that's not already the case. | |||
| let { _parentOrParents } = subscription; | |||
| if (_parentOrParents === null) { | |||
| if (_parentOrParents == null) { | |||
There was a problem hiding this comment.
thought: Yikes. I wonder if there's a lint rule to catch this sort of thing?
There was a problem hiding this comment.
This is intentional. == null matches null or undefined.
There was a problem hiding this comment.
Yeah. I know, your change fixes a potential bug. I'm wondering if comparisons like this could be elsewhere. I know there is a lint rule that relaxes === when comparing null, but IDK that there's one that enforces/recommends the use of == when comparing against null. With the varied use of null and undefined in this codebase - callers can pass either - it would be interesting - to me, anyway - to see if this occurs elsewhere. A search could find it, but a lint rule could enforce it.
There was a problem hiding this comment.
It seems that there are about half a dozen other uses of either === null or === undefined throughout the codebase (ignoring the docs app).
…reference
- Updates a few locations where this feature was being used incorrectly.
- Eliminates confusion where people were trying to "chain" `add` calls.
BREAKING CHANGE: `add` no longer returns an unnecessary Subscription reference. This means that if you are calling `add` with a _function_ and not a `Subscription` (e.g. `subscription.add(() => { /* teardown */ })`), you will not be able to remove that teardown _function_ with `remove`. The fix for this is to wrap your function with a `Subscription` like so: `const childSub = new Subscription(() => { /* teardown */ }); subscription.add(childSub);`. Then you will be able to remove it via `subscription.remove(childSub);`. Bear in mind that is it an edge case to need to manually remove a child subscription from a parent subscription. **All subscriptions that have been added to other subscriptions will remove themselves from the parent subscription(s) automatically when they are unsubscribed.**.
benlesh
force-pushed
the
feat/Subscription-add-returns-void
branch
from
add28f4
to
dd8b8a9
Compare
…reference
addcalls.BREAKING CHANGE:
addno longer returns an unnecessary Subscription reference. This means that if you are callingaddwith a function and not aSubscription(e.g.subscription.add(() => { /* teardown */ })), you will not be able to remove that teardown function withremove. The fix for this is to wrap your function with aSubscriptionlike so:const childSub = new Subscription(() => { /* teardown */ }); subscription.add(childSub);. Then you will be able to remove it viasubscription.remove(childSub);. Bear in mind that is it an edge case to need to manually remove a child subscription from a parent subscription. All subscriptions that have been added to other subscriptions will remove themselves from the parent subscription(s) automatically when they are unsubscribed..