private method subscription

[kurtommy]

HI @NetanelBasal , just a suggestion:

with that decorator you can only check for class variable subscription or class arrays scubscription
if you have something like this

method() {
  const sub = Observable.interval(1000).subscribe(val => {this.val = val})
}

the sub const here is not unsubscribed. You could force the user to put sub inside a class array to being collected by the decorator loop but in this case i'd suggest to not loop trough all class properties but introduce a patter that require to add all the subscription that needs un subscription inside a known variable like

this.subscriptions.push(Observable.interval(1000).subscribe(val => {this.val = val}))

or better inside a named Map() to being able of retrieve the subscription easily.

in the decorator just loop through the subscriptions array and call unsubscribe

[NetanelBasal]

Is your suggestion to drop the support to unsubscribe from something like this?

this.sub = Observable.interval(1000).subscribe()

[kurtommy]

i'm suggesting to put all subscriptions inside a class array called subscriptions

this.subscriptions.push(Observable.interval(1000).subscribe())

or if the user wants to treat like map

this.subscriptions.add('timer', Observable.interval(1000).subscribe())

then in the decorator loop trough the array or map and clean the subscriptions
in this way you don't need to check all the pros of the class, in a complex app for a page change you could delete a lots of components, and iterating through all the props i think is not performant

[NetanelBasal]

In general, you are right, but I don't want to force the array pattern if you have one or two subscriptions it can be annoying.

[NetanelBasal]

What can be done is when you pass the includeArrays option I will know the subscriptions key and loop only over that. That sounds ok?

[kurtommy]

you are already forcing this pattern
instad of this

method() {
  const sub = Observable.interval(1000).subscribe(val => {this.val = val})
}

this

method() {
  this.sub = Observable.interval(1000).subscribe(val => {this.val = val}))
}

so is not much different then this one where you have the best performance

method() {
  this.subscriptions.push( Observable.interval(1000).subscribe(val => {this.val = val})
}

yeah you can change the flag to work on that way

[NetanelBasal]

You do not find this line verbose?

this.subscriptions.push( Observable.interval(1000).subscribe(val => {this.val = val})

[NetanelBasal]

I will change the flag to force a specific property key when you pass includeArrays. You want to make a PR or I will do it when I find the time?

[kurtommy]

this
this.subscriptions.push( Observable.interval(1000).subscribe(val => {this.val = val})
it's very verbose! but it's verbose like using the takeUntil() solution etc...
your automagically solution looks the easiest (considering not using private method vars) but is not fine for perf IMHO, so if you add that flag could be a win win for both scenario and the user can decide which way to use from case to case.