Merge pull request #1135 from NullVoxPopuli/non-block-clean-up-change

Fix issue where, in some situations, the immediate-invoker helper manager (used when you use `resourceFactory`) was not correctly destroying the previous instance of a resource (such as when args change))

Author: NullVoxPopuli

ember-resources/src/function-based/immediate-invocation.ts

@@ -1,7 +1,7 @@
 // @ts-ignore
 import { createCache, getValue } from '@glimmer/tracking/primitives/cache';
 import { assert } from '@ember/debug';
-import { associateDestroyableChild } from '@ember/destroyable';
+import { associateDestroyableChild, destroy } from '@ember/destroyable';
 // @ts-ignore
 import { capabilities as helperCapabilities, invokeHelper, setHelperManager } from '@ember/helper';
 import { dependencySatisfies, importSync, macroCondition } from '@embroider/macros';
@@ -14,9 +14,6 @@ type ResourceFactory<Value = any, Args = any> = (...args: SpreadFor<Args>) => Va
 
 interface State {
   cache: ReturnType<typeof invokeHelper>;
-  fn: any;
-  args: any;
-  _?: any;
 }
 
 let setOwner: (context: unknown, owner: Owner) => void;
@@ -40,6 +37,7 @@ class ResourceInvokerManager {
   constructor(protected owner: Owner) {}
 
   createHelper(fn: ResourceFactory, args: any): State {
+    let previous: object | undefined;
     /**
      * This cache is for args passed to the ResourceInvoker/Factory
      *
@@ -51,12 +49,20 @@ class ResourceInvokerManager {
 
       setOwner(resource, this.owner);
 
-      return invokeHelper(cache, resource);
+      let result = invokeHelper(cache, resource);
+
+      if (previous) {
+        destroy(previous);
+      }
+
+      previous = result;
+
+      return result;
     });
 
     setOwner(cache, this.owner);
 
-    return { fn, args, cache, _: getValue(cache) };
+    return { cache };
   }
 
   /**
@@ -72,6 +78,9 @@ class ResourceInvokerManager {
   }
 
   getDestroyable({ cache }: State) {
+    /**
+     * This is the parent cache, from `createHelper`
+     */
     return cache;
   }
 }

ember-resources/src/function-based/resource.ts

@@ -186,6 +186,7 @@ export function resource<Value>(
     let internalConfig: InternalFunctionResourceConfig<Value> = {
       definition: context as ResourceFunction<Value>,
       type: 'function-based',
+      name: 'Resource',
       [INTERNAL]: true,
     };
 
@@ -222,10 +223,19 @@ export function resource<Value>(
   let internalConfig: InternalFunctionResourceConfig<Value> = {
     definition: setup as ResourceFunction<Value>,
     type: TYPE,
+    name: getDebugName(setup),
     [INTERNAL]: true,
   };
 
   setHelperManager(ResourceManagerFactory, internalConfig);
 
   return wrapForPlainUsage(context, internalConfig);
 }
+
+function getDebugName(obj: object) {
+  if ('name' in obj) {
+    return `Resource Function: ${obj.name}`;
+  }
+
+  return `Resource Function`;
+}

ember-resources/src/function-based/types.ts

@@ -7,6 +7,7 @@ export const INTERNAL = '__INTERNAL__';
 export interface InternalFunctionResourceConfig<Value = unknown> {
   definition: ResourceFunction<Value>;
   type: 'function-based';
+  name: string;
   [INTERNAL]: true;
 }
 

ember-resources/tsconfig.json

@@ -1,6 +1,6 @@
 {
   "extends": "@tsconfig/ember/tsconfig.json",
-  "include": ["src/**/*", "unpublished-development-types/**/*"],
+  "include": ["src/**/*"],
   "glint": {
     "environment": []
   },
@@ -42,15 +42,21 @@
     "allowImportingTsExtensions": true,
 
     // require extensions
-    "module": "Node16",
-    "moduleResolution": "Node16",
+    // "module": "Node16",
+    // "moduleResolution": "Node16",
+    // TODO: enable the above when this package gets to add type=module
+    "moduleResolution": "bundler",
+    "module": "esnext",
+    "target": "esnext",
 
     // https://www.typescriptlang.org/tsconfig#stripInternal
     "stripInternal": true,
 
     // Build settings
     "noEmitOnError": false,
     // Just a good thing to do
-    "isolatedModules": true
+    "isolatedModules": true,
+
+    "types": ["ember-source/types"]
   }
 }

ember-resources/unpublished-development-types/index.d.ts

@@ -1,11 +1,16 @@
 'use strict';
 
+const path = require('path');
 const EmberApp = require('ember-cli/lib/broccoli/ember-app');
 
 const packageJson = require('./package');
 
 module.exports = function (defaults) {
+  const sideWatch = require('@embroider/broccoli-side-watch');
   let app = new EmberApp(defaults, {
+    trees: {
+      app: sideWatch('app', { watching: [path.join(__dirname, '../ember-resources')] }),
+    },
     // Add options here
     autoImport: {
       watchDependencies: Object.keys(packageJson.dependencies),

test-app/ember-cli-build.js

@@ -44,6 +44,7 @@
     "@ember/optional-features": "^2.0.0",
     "@ember/string": "^3.0.1",
     "@ember/test-helpers": "^3.2.1",
+    "@embroider/broccoli-side-watch": "0.0.2-unstable.ba9fd29",
     "@embroider/compat": "^3.1.5",
     "@embroider/core": "^3.1.3",
     "@embroider/test-setup": "^3.0.1",

test-app/package.json

@@ -0,0 +1,64 @@
+import { clearRender,render, settled } from '@ember/test-helpers';
+import { module, test } from 'qunit';
+import { setupRenderingTest } from 'ember-qunit';
+
+import { cell, resource, resourceFactory } from 'ember-resources';
+
+
+module('issues/1134', function(hooks) {
+  setupRenderingTest(hooks);
+
+  test('it works as a vanilla resource', async function (assert) {
+    let value = cell(0);
+
+      const Data = resource(({ on }) => {
+      let arg = value.current;
+
+        assert.step(`setup: ${arg}`);
+        on.cleanup(() => assert.step(`cleanup: ${arg}`));
+
+        return 0;
+      });
+
+    await render(<template>{{Data}}</template>);
+
+    assert.verifySteps(['setup: 0']);
+
+    value.current = 1;
+    await settled();
+    assert.verifySteps(['setup: 1', 'cleanup: 0']);
+
+    await clearRender();
+    assert.verifySteps([`cleanup: ${value.current}`]);
+
+  });
+
+  test('it works with a Wrapper (resourceFactory)', async function (assert) {
+    let value = cell(0);
+
+    function Wrapper(arg: number) {
+      assert.step(`wrapper: ${arg}`);
+
+      return resource(({ on }) => {
+        assert.step(`setup: ${arg}`);
+        on.cleanup(() => assert.step(`cleanup: ${arg}`));
+
+        return 0;
+      });
+    }
+
+    resourceFactory(Wrapper);
+
+    await render(<template>{{Wrapper value.current}}</template>);
+
+    assert.verifySteps(['wrapper: 0', 'setup: 0']);
+
+    value.current = 1;
+    await settled();
+    assert.verifySteps(['wrapper: 1', 'setup: 1', 'cleanup: 0']);
+
+    await clearRender();
+    assert.verifySteps([`cleanup: ${value.current}`]);
+  });
+
+});