Plug memory leak

.changeset/two-meals-drive.md

@@ -0,0 +1,23 @@
+---
+'ember-headless-table': patch
+---
+
+Address an issue where instances of plugins would be held on to after a Table is destroyed.
+
+This caused a memory leak due how plugins, and their associated metadata, held on to
+Table instances, which in turn, held on to the owner / container.
+
+This was caused by the utility methods in `ember-headless-table/plugins`,
+
+- `preferences`
+- `meta`
+- `options`
+
+Because data was stored in (Weak)Maps in module-space.
+This alone isn't a problem, but they were never cleaned up when the table was destroyed.
+
+Cleanup of these objects could have occured via `associateDestroyableChild` and `registerDestructor`
+from `@ember/destroyable`, but it was easier to instead have this happen automatically via hosting the
+data needed for the "plugins utils" on the table itself. Since each plugin util requires "some instance of something",
+be that a row, column, or table, there is a direct path to the table, and therefor a direct way to access
+memory-scoped (Weak)Maps.

ember-headless-table/src/-private/table.ts

@@ -38,6 +38,9 @@ interface Signature<DataType> {
  * ember-resources.
  */
 export const TABLE_KEY = Symbol('__TABLE_KEY__');
+export const TABLE_META_KEY = Symbol('__TABLE_META__');
+export const COLUMN_META_KEY = Symbol('__COLUMN_META__');
+export const ROW_META_KEY = Symbol('__ROW_META__');
 
 const attachContainer = (element: Element, table: Table) => {
   assert('Must be installed on an HTMLElement', element instanceof HTMLElement);
@@ -50,6 +53,18 @@ export class Table<DataType = unknown> extends Resource<Signature<DataType>> {
    * @private
    */
   [TABLE_KEY] = guidFor(this);
+  /**
+   * @private
+   */
+  [TABLE_META_KEY] = new Map<Class<unknown>, any>();
+  /**
+   * @private
+   */
+  [COLUMN_META_KEY] = new WeakMap<Column, Map<Class<unknown>, any>>();
+  /**
+   * @private
+   */
+  [ROW_META_KEY] = new WeakMap<Row, Map<Class<unknown>, any>>();
 
   /**
    * @private

ember-headless-table/src/plugins/-private/base.ts

@@ -1,6 +1,6 @@
 import { assert } from '@ember/debug';
 
-import { TABLE_KEY } from '../../-private/table';
+import { COLUMN_META_KEY, ROW_META_KEY, TABLE_KEY, TABLE_META_KEY } from '../../-private/table';
 import { normalizePluginsConfig } from './utils';
 
 import type { Table } from '../../-private/table';
@@ -17,10 +17,6 @@ import type {
   TableMetaFor,
 } from '#interfaces';
 
-const TABLE_META = new Map<string, Map<Class<unknown>, any>>();
-const COLUMN_META = new WeakMap<Column, Map<Class<unknown>, any>>();
-const ROW_META = new WeakMap<Row, Map<Class<unknown>, any>>();
-
 type InstanceOf<T> = T extends Class<infer Instance> ? Instance : T;
 
 /**
@@ -445,7 +441,9 @@ export const meta = {
     column: Column<Data>,
     klass: Class<P>
   ): ColumnMetaFor<SignatureFrom<P>> {
-    return getPluginInstance(COLUMN_META, column, klass, () => {
+    let columnMeta = column.table[COLUMN_META_KEY];
+
+    return getPluginInstance(columnMeta, column, klass, () => {
       let plugin = column.table.pluginOf(klass);
 
       assert(`[${klass.name}] cannot get plugin instance of unregistered plugin class`, plugin);
@@ -468,7 +466,9 @@ export const meta = {
     row: Row<Data>,
     klass: Class<P>
   ): RowMetaFor<SignatureFrom<P>> {
-    return getPluginInstance(ROW_META, row, klass, () => {
+    let rowMeta = row.table[ROW_META_KEY];
+
+    return getPluginInstance(rowMeta, row, klass, () => {
       let plugin = row.table.pluginOf(klass);
 
       assert(`[${klass.name}] cannot get plugin instance of unregistered plugin class`, plugin);
@@ -489,7 +489,9 @@ export const meta = {
     table: Table<Data>,
     klass: Class<P>
   ): TableMetaFor<SignatureFrom<P>> {
-    return getPluginInstance(TABLE_META, table[TABLE_KEY], klass, () => {
+    let tableMeta = table[TABLE_META_KEY];
+
+    return getPluginInstance(tableMeta, klass, () => {
       let plugin = table.pluginOf(klass);
 
       assert(`[${klass.name}] cannot get plugin instance of unregistered plugin class`, plugin);
@@ -498,7 +500,7 @@ export const meta = {
       assert(
         `<#${plugin.name}> plugin already exists for the table. ` +
           `A plugin may only be instantiated once per table.`,
-        ![...(TABLE_META.get(table[TABLE_KEY])?.keys() ?? [])].includes(klass)
+        ![...tableMeta.keys()].includes(klass)
       );
 
       return new plugin.meta.table(table);
@@ -639,21 +641,55 @@ export const options = {
   },
 };
 
+type FactoryMap<Instance> = Map<Class<Instance>, Instance>;
+
 /**
  * @private
  */
-function getPluginInstance<RootKey extends string | Column<any> | Row<any>, Instance>(
-  map: RootKey extends string
-    ? Map<string, Map<Class<Instance>, Instance>>
-    : WeakMap<Column | Row, Map<Class<Instance>, Instance>>,
+function getPluginInstance<Instance>(
+  map: Map<Class<Instance>, Instance>,
+  mapKey: Class<Instance>,
+  factory: () => Instance
+): Instance;
+function getPluginInstance<RootKey extends Column<any> | Row<any>, Instance>(
+  map: WeakMap<Column | Row, Map<Class<Instance>, Instance>>,
   rootKey: RootKey,
   mapKey: Class<Instance>,
   factory: () => Instance
+): Instance;
+function getPluginInstance<RootKey extends Column<any> | Row<any>, Instance>(
+  ...args:
+    | [FactoryMap<Instance>, Class<Instance>, () => Instance]
+    | [WeakMap<Column | Row, FactoryMap<Instance>>, RootKey, Class<Instance>, () => Instance]
 ): Instance {
-  let bucket: Map<Class<Instance>, Instance> | undefined;
+  let map: WeakMap<Column | Row, FactoryMap<Instance>> | FactoryMap<Instance>;
+  let mapKey: Class<Instance>;
+  let rootKey: RootKey | undefined;
+  let factory: () => Instance;
+
+  if (args.length === 3) {
+    map = args[0];
+    mapKey = args[1];
+    factory = args[2];
+  } else if (args.length === 4) {
+    map = args[0];
+    rootKey = args[1];
+    mapKey = args[2];
+    factory = args[3];
+  } else {
+    throw new Error(
+      // TS says args is of type "never", but TS can't protect against general misuse
+      // (esp without TS)
+      `Incorrect arity passed to getPluginInstance. Expected 3 or 4, received ${
+        (args as any).length
+      }`
+    );
+  }
+
+  let bucket: FactoryMap<Instance> | undefined;
 
   if (map instanceof WeakMap) {
-    assert(`Cannot use string key with WeakMap`, typeof rootKey !== 'string');
+    assert(`rootKey is missing`, rootKey);
 
     bucket = map.get(rootKey);
 
@@ -663,14 +699,7 @@ function getPluginInstance<RootKey extends string | Column<any> | Row<any>, Inst
       map.set(rootKey, bucket);
     }
   } else {
-    assert(`Cannot use object key with Map`, typeof rootKey === 'string');
-    bucket = map.get(rootKey);
-
-    if (!bucket) {
-      bucket = new Map();
-
-      map.set(rootKey, bucket);
-    }
+    bucket = map;
   }
 
   let instance = bucket.get(mapKey);

test-app/ember-cli-build.js

@@ -10,6 +10,9 @@ module.exports = function (defaults) {
     'ember-cli-babel': {
       enableTypeScriptTransform: true,
     },
+    'ember-cli-memory-leak-detector': {
+      enabled: process.env.DETECT_MEMORY_LEAKS || false,
+    },
   });
 
   const { maybeEmbroider } = require('@embroider/test-setup');