refactor(ivy): move static parts of LView.cleanup to TView (#24301)

PR Close #24301

packages/core/src/render3/instructions.ts

@@ -154,30 +154,14 @@ let data: any[];
  */
 let directives: any[]|null;
 
-/**
+function getCleanup(view: LView): any[] {
- * When a view is destroyed, listeners need to be released and outputs need to be
- * unsubscribed. This cleanup array stores both listener data (in chunks of 4)
- * and output data (in chunks of 2) for a particular view. Combining the arrays
- * saves on memory (70 bytes per array) and on a few bytes of code size (for two
- * separate for loops).
- *
- * If it's a listener being stored:
- * 1st index is: event name to remove
- * 2nd index is: native element
- * 3rd index is: listener function
- * 4th index is: useCapture boolean
- *
- * If it's an output subscription:
- * 1st index is: unsubscribe function
- * 2nd index is: context for function
- */
-let cleanup: any[]|null;
-
-export function getCleanup(): any[] {
   // top level variables should not be exported for performance reasons (PERF_NOTES.md)
-  return cleanup || (cleanup = currentView.cleanup = []);
+  return view.cleanupInstances || (view.cleanupInstances = []);
 }
 
+function getTViewCleanup(view: LView): any[] {
+  return view.tView.cleanup || (view.tView.cleanup = []);
+}
 /**
  * In this mode, any changes in bindings will throw an ExpressionChangedAfterChecked error.
  *
@@ -213,7 +197,6 @@ export function enterView(newView: LView, host: LElementNode | LViewNode | null)
   creationMode = newView && (newView.flags & LViewFlags.CreationMode) === LViewFlags.CreationMode;
   firstTemplatePass = newView && newView.tView.firstTemplatePass;
 
-  cleanup = newView && newView.cleanup;
   renderer = newView && newView.renderer;
 
   if (host != null) {
@@ -312,7 +295,7 @@ export function createLView<T>(
     data: [],
     directives: null,
     tView: tView,
-    cleanup: null,
+    cleanupInstances: null,
     renderer: renderer,
     tail: null,
     next: null,
@@ -818,6 +801,7 @@ export function createTView(
     viewCheckHooks: null,
     destroyHooks: null,
     pipeDestroyHooks: null,
+    cleanup: null,
     hostBindings: null,
     components: null,
     directiveRegistry: typeof directives === 'function' ? directives() : directives,
@@ -915,19 +899,23 @@ export function listener(
   ngDevMode && assertPreviousIsParent();
   const node = previousOrParentNode;
   const native = node.native as RElement;
+  ngDevMode && ngDevMode.rendererAddEventListener++;
 
   // In order to match current behavior, native DOM event listeners must be added for all
   // events (including outputs).
-  const cleanupFns = getCleanup();
-  ngDevMode && ngDevMode.rendererAddEventListener++;
   if (isProceduralRenderer(renderer)) {
     const wrappedListener = wrapListenerWithDirtyLogic(currentView, listenerFn);
     const cleanupFn = renderer.listen(native, eventName, wrappedListener);
-    cleanupFns.push(cleanupFn, null);
+    storeCleanupFn(currentView, cleanupFn);
   } else {
     const wrappedListener = wrapListenerWithDirtyAndDefault(currentView, listenerFn);
     native.addEventListener(eventName, wrappedListener, useCapture);
-    cleanupFns.push(eventName, native, wrappedListener, useCapture);
+    const cleanupInstances = getCleanup(currentView);
+    cleanupInstances.push(wrappedListener);
+    if (firstTemplatePass) {
+      getTViewCleanup(currentView)
+          .push(eventName, node.tNode.index, cleanupInstances !.length - 1, useCapture);
+    }
   }
 
   let tNode: TNode|null = node.tNode;
@@ -952,7 +940,39 @@ function createOutput(outputs: PropertyAliasValue, listener: Function): void {
   for (let i = 0; i < outputs.length; i += 2) {
     ngDevMode && assertDataInRange(outputs[i] as number, directives !);
     const subscription = directives ![outputs[i] as number][outputs[i + 1]].subscribe(listener);
-    getCleanup().push(subscription.unsubscribe, subscription);
+    storeCleanupWithContext(currentView, subscription, subscription.unsubscribe);
+  }
+}
+
+/**
+ * Saves context for this cleanup function in LView.cleanupInstances.
+ *
+ * On the first template pass, saves in TView:
+ * - Cleanup function
+ * - Index of context we just saved in LView.cleanupInstances
+ */
+export function storeCleanupWithContext(
+    view: LView = currentView, context: any, cleanupFn: Function): void {
+  getCleanup(view).push(context);
+
+  if (view.tView.firstTemplatePass) {
+    getTViewCleanup(view).push(cleanupFn, view.cleanupInstances !.length - 1);
+  }
+}
+
+/**
+ * Saves the cleanup function itself in LView.cleanupInstances.
+ *
+ * This is necessary for functions that are wrapped with their contexts, like in renderer2
+ * listeners.
+ *
+ * On the first template pass, the index of the cleanup function is saved in TView.
+ */
+export function storeCleanupFn(view: LView, cleanupFn: Function): void {
+  getCleanup(view).push(cleanupFn);
+
+  if (view.tView.firstTemplatePass) {
+    getTViewCleanup(view).push(view.cleanupInstances !.length - 1, null);
   }
 }
 

packages/core/src/render3/interfaces/view.ts

@@ -63,22 +63,14 @@ export interface LView {
 
   /**
    * When a view is destroyed, listeners need to be released and outputs need to be
-   * unsubscribed. This cleanup array stores both listener data (in chunks of 4)
+   * unsubscribed. This context array stores both listener functions wrapped with
-   * and output data (in chunks of 2) for a particular view. Combining the arrays
+   * their context and output subscription instances for a particular view.
-   * saves on memory (70 bytes per array) and on a few bytes of code size (for two
-   * separate for loops).
    *
-   * If it's a listener being stored:
+   * These change per LView instance, so they cannot be stored on TView. Instead,
-   * 1st index is: event name to remove
+   * TView.cleanup saves an index to the necessary context in this array.
-   * 2nd index is: native element
-   * 3rd index is: listener function
-   * 4th index is: useCapture boolean
-   *
-   * If it's an output subscription:
-   * 1st index is: unsubscribe function
-   * 2nd index is: context for function
    */
-  cleanup: any[]|null;
+  // TODO: collapse into data[]
+  cleanupInstances: any[]|null;
 
   /**
    * The last LView or LContainer beneath this LView in the hierarchy.
@@ -368,6 +360,29 @@ export interface TView {
    */
   pipeDestroyHooks: HookData|null;
 
+  /**
+   * When a view is destroyed, listeners need to be released and outputs need to be
+   * unsubscribed. This cleanup array stores both listener data (in chunks of 4)
+   * and output data (in chunks of 2) for a particular view. Combining the arrays
+   * saves on memory (70 bytes per array) and on a few bytes of code size (for two
+   * separate for loops).
+   *
+   * If it's a native DOM listener being stored:
+   * 1st index is: event name to remove
+   * 2nd index is: index of native element in LView.data[]
+   * 3rd index is: index of wrapped listener function in LView.cleanupInstances[]
+   * 4th index is: useCapture boolean
+   *
+   * If it's a renderer2 style listener or ViewRef destroy hook being stored:
+   * 1st index is: index of the cleanup function in LView.cleanupInstances[]
+   * 2nd index is: null
+   *
+   * If it's an output subscription or query list destroy hook:
+   * 1st index is: output unsubscribe function / query list destroy function
+   * 2nd index is: index of function context in LView.cleanupInstances[]
+   */
+  cleanup: any[]|null;
+
   /**
    * A list of directive and element indices for child components that will need to be
    * refreshed when the current view has finished its check.

packages/core/src/render3/node_manipulation.ts

@@ -297,7 +297,7 @@ export function destroyViewTree(rootView: LView): void {
     } else if (viewOrContainer.next) {
       // Only move to the side and clean if operating below rootView -
       // otherwise we would start cleaning up sibling views of the rootView.
-      cleanUpView(viewOrContainer as LView);
+      cleanUpView(viewOrContainer);
       next = viewOrContainer.next;
     }
 
@@ -306,10 +306,10 @@ export function destroyViewTree(rootView: LView): void {
       // with a root view that doesn't have children. We didn't descend into child views
       // so no need to go back up the views tree.
       while (viewOrContainer && !viewOrContainer !.next && viewOrContainer !== rootView) {
-        cleanUpView(viewOrContainer as LView);
+        cleanUpView(viewOrContainer);
         viewOrContainer = getParentState(viewOrContainer, rootView);
       }
-      cleanUpView(viewOrContainer as LView || rootView);
+      cleanUpView(viewOrContainer || rootView);
 
       next = viewOrContainer && viewOrContainer.next;
     }
@@ -472,30 +472,42 @@ export function getParentState(state: LViewOrLContainer, rootView: LView): LView
  *
  * @param view The LView to clean up
  */
-function cleanUpView(view: LView): void {
+function cleanUpView(viewOrContainer: LViewOrLContainer): void {
-  removeListeners(view);
+  if ((viewOrContainer as LView).tView) {
-  executeOnDestroys(view);
+    const view = viewOrContainer as LView;
-  executePipeOnDestroys(view);
+    removeListeners(view);
-  // For component views only, the local renderer is destroyed as clean up time.
+    executeOnDestroys(view);
-  if (view.tView && view.tView.id === -1 && isProceduralRenderer(view.renderer)) {
+    executePipeOnDestroys(view);
-    ngDevMode && ngDevMode.rendererDestroy++;
+    // For component views only, the local renderer is destroyed as clean up time.
-    view.renderer.destroy();
+    if (view.tView.id === -1 && isProceduralRenderer(view.renderer)) {
+      ngDevMode && ngDevMode.rendererDestroy++;
+      view.renderer.destroy();
+    }
   }
 }
 
 /** Removes listeners and unsubscribes from output subscriptions */
 function removeListeners(view: LView): void {
-  const cleanup = view.cleanup !;
+  const cleanup = view.tView.cleanup !;
   if (cleanup != null) {
     for (let i = 0; i < cleanup.length - 1; i += 2) {
       if (typeof cleanup[i] === 'string') {
-        cleanup ![i + 1].removeEventListener(cleanup[i], cleanup[i + 2], cleanup[i + 3]);
+        // This is a listener with the native renderer
+        const native = view.data[cleanup[i + 1]].native;
+        const listener = view.cleanupInstances ![cleanup[i + 2]];
+        native.removeEventListener(cleanup[i], listener, cleanup[i + 3]);
         i += 2;
+      } else if (typeof cleanup[i] === 'number') {
+        // This is a listener with renderer2 (cleanup fn can be found by index)
+        const cleanupFn = view.cleanupInstances ![cleanup[i]];
+        cleanupFn();
       } else {
-        cleanup[i].call(cleanup[i + 1]);
+        // This is a cleanup function that is grouped with the index of its context
+        const context = view.cleanupInstances ![cleanup[i + 1]];
+        cleanup[i].call(context);
       }
     }
-    view.cleanup = null;
+    view.cleanupInstances = null;
   }
 }
 

packages/core/src/render3/query.ts

@@ -17,7 +17,7 @@ import {getSymbolIterator} from '../util';
 
 import {assertEqual, assertNotNull} from './assert';
 import {ReadFromInjectorFn, getOrCreateNodeInjectorForNode} from './di';
-import {assertPreviousIsParent, getCleanup, getCurrentQueries, store} from './instructions';
+import {assertPreviousIsParent, getCurrentQueries, store, storeCleanupWithContext} from './instructions';
 import {DirectiveDef, unusedValueExportToPlacateAjd as unused1} from './interfaces/definition';
 import {LInjector, unusedValueExportToPlacateAjd as unused2} from './interfaces/injector';
 import {LContainerNode, LElementNode, LNode, TNode, TNodeFlags, unusedValueExportToPlacateAjd as unused3} from './interfaces/node';
@@ -416,7 +416,7 @@ export function query<T>(
   const queryList = new QueryList<T>();
   const queries = getCurrentQueries(LQueries_);
   queries.track(queryList, predicate, descend, read);
-  getCleanup().push(queryList.destroy, queryList);
+  storeCleanupWithContext(undefined, queryList, queryList.destroy);
   if (memoryIndex != null) {
     store(memoryIndex, queryList);
   }

packages/core/src/render3/view_ref.ts

@@ -9,7 +9,7 @@
 import {ViewContainerRef as viewEngine_ViewContainerRef} from '../linker/view_container_ref';
 import {EmbeddedViewRef as viewEngine_EmbeddedViewRef} from '../linker/view_ref';
 
-import {checkNoChanges, detectChanges, markViewDirty} from './instructions';
+import {checkNoChanges, detectChanges, markViewDirty, storeCleanupFn} from './instructions';
 import {ComponentTemplate} from './interfaces/definition';
 import {LViewNode} from './interfaces/node';
 import {LView, LViewFlags} from './interfaces/view';
@@ -33,9 +33,7 @@ export class ViewRef<T> implements viewEngine_EmbeddedViewRef<T> {
 
   destroy(): void { destroyLView(this._view); }
 
-  onDestroy(callback: Function) {
+  onDestroy(callback: Function) { storeCleanupFn(this._view, callback); }
-    (this._view.cleanup || (this._view.cleanup = [])).push(callback, null);
-  }
 
   /**
    * Marks a view and all of its ancestors dirty.

packages/core/test/bundling/todo/bundle.golden_symbols.json

@@ -443,6 +443,9 @@
   {
     "name": "getSymbolIterator"
   },
+  {
+    "name": "getTViewCleanup"
+  },
   {
     "name": "getTypeNameForDebugging"
   },
@@ -626,6 +629,12 @@
   {
     "name": "setUpAttributes"
   },
+  {
+    "name": "storeCleanupFn"
+  },
+  {
+    "name": "storeCleanupWithContext"
+  },
   {
     "name": "stringify"
   },

packages/core/test/render3/listeners_spec.ts

@@ -10,7 +10,7 @@ import {defineComponent, defineDirective} from '../../src/render3/index';
 import {container, containerRefreshEnd, containerRefreshStart, elementEnd, elementStart, embeddedViewEnd, embeddedViewStart, listener, text} from '../../src/render3/instructions';
 import {RenderFlags} from '../../src/render3/interfaces/definition';
 import {getRendererFactory2} from './imported_renderer2';
-import {containerEl, renderComponent, renderToHtml} from './render_util';
+import {ComponentFixture, containerEl, renderComponent, renderToHtml} from './render_util';
 
 
 describe('event listeners', () => {
@@ -228,6 +228,187 @@ describe('event listeners', () => {
     expect(comp.counter).toEqual(2);
   });
 
+  it('should destroy listeners in views with renderer2', () => {
+
+    /**
+     * % if (ctx.showing) {
+       *  <button (click)="onClick()"> Click me </button>
+       * % }
+     */
+    class AppComp {
+      counter = 0;
+      showing = true;
+
+      onClick() { this.counter++; }
+
+      static ngComponentDef = defineComponent({
+        type: AppComp,
+        selectors: [['app-comp']],
+        factory: () => new AppComp(),
+        template: function(rf: RenderFlags, ctx: any) {
+          if (rf & RenderFlags.Create) {
+            container(0);
+          }
+          if (rf & RenderFlags.Update) {
+            containerRefreshStart(0);
+            {
+              if (ctx.showing) {
+                if (embeddedViewStart(0)) {
+                  elementStart(0, 'button');
+                  {
+                    listener('click', function() { return ctx.onClick(); });
+                    text(1, 'Click me');
+                  }
+                  elementEnd();
+                }
+                embeddedViewEnd();
+              }
+            }
+            containerRefreshEnd();
+          }
+        }
+      });
+    }
+
+    const fixture = new ComponentFixture(AppComp, {rendererFactory: getRendererFactory2(document)});
+    const comp = fixture.component;
+    const button = fixture.hostElement.querySelector('button') !;
+
+    button.click();
+    expect(comp.counter).toEqual(1);
+
+    button.click();
+    expect(comp.counter).toEqual(2);
+
+    // the listener should be removed when the view is removed
+    comp.showing = false;
+    fixture.update();
+    button.click();
+    expect(comp.counter).toEqual(2);
+  });
+
+  it('should destroy listeners in for loops', () => {
+
+    /**
+     * % for (let i = 0; i < ctx.buttons; i++) {
+       *  <button (click)="onClick(i)"> Click me </button>
+       * % }
+     */
+    class AppComp {
+      buttons = 2;
+      counters = [0, 0];
+
+      onClick(index: number) { this.counters[index]++; }
+
+      static ngComponentDef = defineComponent({
+        type: AppComp,
+        selectors: [['app-comp']],
+        factory: () => new AppComp(),
+        template: function(rf: RenderFlags, ctx: any) {
+          if (rf & RenderFlags.Create) {
+            container(0);
+          }
+          if (rf & RenderFlags.Update) {
+            containerRefreshStart(0);
+            {
+              for (let i = 0; i < ctx.buttons; i++) {
+                if (embeddedViewStart(0)) {
+                  elementStart(0, 'button');
+                  {
+                    listener('click', function() { return ctx.onClick(i); });
+                    text(1, 'Click me');
+                  }
+                  elementEnd();
+                }
+                embeddedViewEnd();
+              }
+            }
+            containerRefreshEnd();
+          }
+        }
+      });
+    }
+
+    const fixture = new ComponentFixture(AppComp);
+    const comp = fixture.component;
+    const buttons = fixture.hostElement.querySelectorAll('button') !;
+
+    buttons[0].click();
+    expect(comp.counters).toEqual([1, 0]);
+
+    buttons[1].click();
+    expect(comp.counters).toEqual([1, 1]);
+
+    // the listener should be removed when the view is removed
+    comp.buttons = 0;
+    fixture.update();
+
+    buttons[0].click();
+    buttons[1].click();
+    expect(comp.counters).toEqual([1, 1]);
+  });
+
+  it('should destroy listeners in for loops with renderer2', () => {
+
+    /**
+     * % for (let i = 0; i < ctx.buttons; i++) {
+       *  <button (click)="onClick(i)"> Click me </button>
+       * % }
+     */
+    class AppComp {
+      buttons = 2;
+      counters = [0, 0];
+
+      onClick(index: number) { this.counters[index]++; }
+
+      static ngComponentDef = defineComponent({
+        type: AppComp,
+        selectors: [['app-comp']],
+        factory: () => new AppComp(),
+        template: function(rf: RenderFlags, ctx: any) {
+          if (rf & RenderFlags.Create) {
+            container(0);
+          }
+          if (rf & RenderFlags.Update) {
+            containerRefreshStart(0);
+            {
+              for (let i = 0; i < ctx.buttons; i++) {
+                if (embeddedViewStart(1)) {
+                  elementStart(0, 'button');
+                  {
+                    listener('click', function() { return ctx.onClick(i); });
+                    text(1, 'Click me');
+                  }
+                  elementEnd();
+                }
+                embeddedViewEnd();
+              }
+            }
+            containerRefreshEnd();
+          }
+        }
+      });
+    }
+
+    const fixture = new ComponentFixture(AppComp, {rendererFactory: getRendererFactory2(document)});
+    const comp = fixture.component;
+    const buttons = fixture.hostElement.querySelectorAll('button') !;
+
+    buttons[0].click();
+    expect(comp.counters).toEqual([1, 0]);
+
+    buttons[1].click();
+    expect(comp.counters).toEqual([1, 1]);
+
+    // the listener should be removed when the view is removed
+    comp.buttons = 0;
+    fixture.update();
+
+    buttons[0].click();
+    buttons[1].click();
+    expect(comp.counters).toEqual([1, 1]);
+  });
+
   it('should support host listeners', () => {
     let events: string[] = [];
 

packages/core/test/render3/render_util.ts

@@ -103,7 +103,7 @@ export class ComponentFixture<T> extends BaseFixture {
 
   constructor(
       private componentType: ComponentType<T>,
-      opts: {injector?: Injector, sanitizer?: Sanitizer} = {}) {
+      opts: {injector?: Injector, sanitizer?: Sanitizer, rendererFactory?: RendererFactory3} = {}) {
     super();
     this.requestAnimationFrame = function(fn: () => void) {
       requestAnimationFrame.queue.push(fn);
@@ -119,7 +119,8 @@ export class ComponentFixture<T> extends BaseFixture {
       host: this.hostElement,
       scheduler: this.requestAnimationFrame,
       injector: opts.injector,
-      sanitizer: opts.sanitizer
+      sanitizer: opts.sanitizer,
+      rendererFactory: opts.rendererFactory || domRendererFactory3
     });
   }