ngneat / until-destroy

🦊 RxJS operator that unsubscribe from observables on destroy
https://netbasal.com/
MIT License
1.74k stars 100 forks source link

Angular 16 takeUntilDestroyed #233

Closed muuvmuuv closed 10 months ago

muuvmuuv commented 1 year ago

Just a technical question: are the implementations different from this pipe and Angular 16 newest rxjs-interop takeUntilDestroyed? I can see that NG#s version does not need (this), so I am unsure.

Or, had you merged your outstanding work here into the Angular project?

arturovt commented 1 year ago

Or, had you merged your outstanding work here into the Angular project?

Haha, nope

Implementations are different from a technical perspective since takeUntilDestroyed relies on the DestroyRef injection unit. You must always provide it when calling the operator outside of the injection context (e.g., ngOnInit).

In my large codebases, I used untilDestroyed(this) over 1000 times, but I started using takeUntilDestroyed after upgrading to Angular 16. However, I still use untilDestroyed(this) in services since takeUntilDestroyed can only be used in components and directives (where DestroyRef is available).

muuvmuuv commented 1 year ago

Ah, alright! Thanks for the details. I just tested to migrate a few components, and it is a little tricky. :D

takeUntilDestroyed() can only be used within an injection context such as a constructor, a factory function, a field initializer, or a function used with `runInInjectionContext`. Find more at https://angular.io/errors/NG0203
Error: NG0203: takeUntilDestroyed() can only be used within an injection context such as a constructor, a factory function, a field initializer, or a function used with `runInInjectionContext`. Find more at https://angular.io/errors/NG0203

Considering performance when talking about 1000x (we have a little less), would you say one is better tho “until-destroy” hooks into the constructor and has “one more” import? Or in your case, why did you choose to use takeUntilDestroyed anyway, just to reduce boilerplate?

Just trying to figure out which way we go, especially when Signals are coming, I guess that takeUntilDestroyed is kind of developed for the new component api/way NG moving to.

muuvmuuv commented 1 year ago

I see there was already a try on until-destroy side to make something like that, in #122.

arturovt commented 1 year ago

The takeUntilDestroyed requires DestroyRef outside of the context:

class App {
  destroyRef = inject(DestroyRef);

  constructor() {
    // Can be used w/o arguments.
    of(true).pipe(takeUntilDestroyed()).subscribe();
  }

  ngOnInit() {
    // Requires `DestroyRef`
    of(true).pipe(takeUntilDestroyed(this.destroyRef)).subscribe();
  }
}
arturovt commented 1 year ago

I don't think there's anything related to performance in this case since both operators are essentially piping source observables with takeUntil(destroySubject) internally. However, using the decorator forces the compiler to emit different code. You can see more details about this issue in this link: https://github.com/ngneat/until-destroy/issues/232#issuecomment-1589762258

muuvmuuv commented 1 year ago

Thanks for the example. OK, sounds like in general it would be a good "best-practise" to move to inject(DestroyRef), since Angular is moving towards that syntax and our codebase is almost only standalone components. Ah, and as well to cover #232.

devfservant commented 11 months ago

since takeUntilDestroyed can only be used in components and directives (where DestroyRef is available)

I don't understand: from the docs, DestroyRef may be injected elsewhere than in components and directives, no?
From my understanding, any untilDestroyed(this) is replaceable by a takeUntilDestroyed(this.destroyRef).
Or would I miss something?