refactor(multiple): use renderer for manual event bindings #30179
Conversation
crisbeto
force-pushed
the
events-renderer
branch
2 times, most recently
from
238bfd0 to
d004e3a
Compare
| this._hostElement.addEventListener('pointermove', this._onPointerMove.bind(this)); | ||
| this._hostElement.addEventListener('pointerup', this._onPointerUp.bind(this)); | ||
| this._listenerCleanups = [ | ||
| renderer.listen(this._hostElement, 'pointerdown', this._onPointerDown.bind(this)), |
There was a problem hiding this comment.
Note that this one actually fixes a memory leak. Previously the events wouldn't have been removed, because bind returns a new function instead and for the removeEventListener call to work below, it has to be the same function.
| private _ngZone = inject(NgZone); | ||
| private _viewportRuler = inject(ViewportRuler); | ||
| private _dragDropRegistry = inject(DragDropRegistry); | ||
| private _renderer = inject(RendererFactory2).createRenderer(null, null); |
There was a problem hiding this comment.
Note that the renderer can't be injected in services so we have to go through the factory. Even though the method is called createRenderer, the service will return the same renderer every time: https://github.com/angular/angular/blob/main/packages/platform-browser/src/dom/dom_renderer.ts#L117
There was a problem hiding this comment.
This seems a little unfortunate since it relies on the implementation in the framework. Would be nice (and out of scope) if Angular had some better supported API for the case of a servicing wanting the default renderer. As it reads, passing null, null is confusing and doesnt even seem like it should be valid
crisbeto
requested review from
andrewseguin and
mmalerba
and removed request for
a team
There was a problem hiding this comment.
Weird that the docs for listen don't provide jsdoc information for @returns
| private _ngZone = inject(NgZone); | ||
| private _viewportRuler = inject(ViewportRuler); | ||
| private _dragDropRegistry = inject(DragDropRegistry); | ||
| private _renderer = inject(RendererFactory2).createRenderer(null, null); |
There was a problem hiding this comment.
This seems a little unfortunate since it relies on the implementation in the framework. Would be nice (and out of scope) if Angular had some better supported API for the case of a servicing wanting the default renderer. As it reads, passing null, null is confusing and doesnt even seem like it should be valid
src/cdk/overlay/overlay-ref.ts
Outdated
| /** Removes a backdrop element from the DOM. */ | ||
| private _disposeBackdrop(backdrop: HTMLElement | null) { | ||
| this._cleanupBackdropClick?.(); | ||
| this._cleanupBackdropClick?.(); |
There was a problem hiding this comment.
should be _cleanupBackdropTransitionEnd?
Switches our manual calls into `addEventListener` to use the renderer instead so that tooling (e.g. the tracing service) gets notified about them. Note that these changes only include the events that don't pass event options. The remaining events will be changed in a follow-up, because they require us to update to the latest `next` version of Angular.
crisbeto
force-pushed
the
events-renderer
branch
from
d004e3a to
f9a638f
Compare
Switches our manual calls into `addEventListener` to use the renderer instead so that tooling (e.g. the tracing service) gets notified about them. Note that these changes only include the events that don't pass event options. The remaining events will be changed in a follow-up, because they require us to update to the latest `next` version of Angular. (cherry picked from commit 04e5e0f)
In angular/components#30179 the CDK overlay started depending on the `Renderer2Factory`. Since the overlay is used in the `MatSnackbar` which is commonly used in error handlers, `Overlay` can end up being injected as a part of the app initialization. Because `AsyncAnimationRendererFactory` depends on the `ChangeDetectionScheduler`, it may cause a circular dependency. These changes inject the `ChangeDetectionScheduler` lazily to avoid the error. Note: this will also be resolved by angular#58984, but I decided to send it out, because: 1. angular#58984 seems to be stuck on some internal cleanup. 2. The `AsyncAnimationRendererFactory` doesn't need the `scheduler` eagerly anyway so the change is fairly safe. Fixes angular#59255.
) In angular/components#30179 the CDK overlay started depending on the `Renderer2Factory`. Since the overlay is used in the `MatSnackbar` which is commonly used in error handlers, `Overlay` can end up being injected as a part of the app initialization. Because `AsyncAnimationRendererFactory` depends on the `ChangeDetectionScheduler`, it may cause a circular dependency. These changes inject the `ChangeDetectionScheduler` lazily to avoid the error. Note: this will also be resolved by #58984, but I decided to send it out, because: 1. #58984 seems to be stuck on some internal cleanup. 2. The `AsyncAnimationRendererFactory` doesn't need the `scheduler` eagerly anyway so the change is fairly safe. Fixes #59255. PR Close #59256
In angular/components#30179 the CDK overlay started depending on the `Renderer2Factory`. Since the overlay is used in the `MatSnackbar` which is commonly used in error handlers, `Overlay` can end up being injected as a part of the app initialization. Because `AsyncAnimationRendererFactory` depends on the `ChangeDetectionScheduler`, it may cause a circular dependency. These changes inject the `ChangeDetectionScheduler` lazily to avoid the error. Note: this will also be resolved by angular#58984, but I decided to send it out, because: 1. angular#58984 seems to be stuck on some internal cleanup. 2. The `AsyncAnimationRendererFactory` doesn't need the `scheduler` eagerly anyway so the change is fairly safe. Fixes angular#59255.
) In angular/components#30179 the CDK overlay started depending on the `Renderer2Factory`. Since the overlay is used in the `MatSnackbar` which is commonly used in error handlers, `Overlay` can end up being injected as a part of the app initialization. Because `AsyncAnimationRendererFactory` depends on the `ChangeDetectionScheduler`, it may cause a circular dependency. These changes inject the `ChangeDetectionScheduler` lazily to avoid the error. Note: this will also be resolved by #58984, but I decided to send it out, because: 1. #58984 seems to be stuck on some internal cleanup. 2. The `AsyncAnimationRendererFactory` doesn't need the `scheduler` eagerly anyway so the change is fairly safe. Fixes #59255. PR Close #59271
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
2 participants
Switches our manual calls into
addEventListenerto use the renderer instead so that tooling (e.g. the tracing service) gets notified about them.Note that these changes only include the events that don't pass event options. The remaining events will be changed in a follow-up, because they require us to update to the latest
nextversion of Angular.