Merge pull request #1323 from glimmerjs/make-hashes-lazy-in-templates…

…-only Revert hash to bi-modal behavior
glimmerjs · Jun 29, 2021 · 9c44e68 · 9c44e68
2 parents 5e01d39 + 52f5257
commit 9c44e68
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 155 deletions.
diff --git a/packages/@glimmer/integration-tests/test/helpers/hash-test.ts b/packages/@glimmer/integration-tests/test/helpers/hash-test.ts
@@ -189,6 +189,8 @@ class HashTest extends RenderTest {
       constructor(owner: object, args: { hash: Record<string, unknown> }) {
         super(owner, args);
         args.hash.firstName = 'Chad';
+
+        assert.equal(args.hash.firstName, 'Chad', 'Name updated in JS');
       }
     }
 
@@ -198,12 +200,16 @@ class HashTest extends RenderTest {
       `<FooBar @hash={{hash firstName="Godfrey" lastName="Hietala"}} as |values|>{{values.firstName}} {{values.lastName}}</FooBar>`
     );
 
-    this.assertHTML('Chad Hietala');
+    // Name will not be updated in templates because templates access the child
+    // reference on hashes directly
+    this.assertHTML('Godfrey Hietala');
     this.assertStableRerender();
 
-    assert.validateDeprecations(
-      /You set the 'firstName' property on a {{hash}} object. Setting properties on objects generated by {{hash}} is deprecated. Please update to use an object created with a tracked property or getter, or with a custom helper./
-    );
+    if (HAS_NATIVE_PROXY) {
+      assert.validateDeprecations(
+        /You set the 'firstName' property on a {{hash}} object. Setting properties on objects generated by {{hash}} is deprecated. Please update to use an object created with a tracked property or getter, or with a custom helper./
+      );
+    }
   }
 
   @test
@@ -213,25 +219,36 @@ class HashTest extends RenderTest {
         super(owner, args);
         args.hash.name = 'Chad';
       }
+
+      get alias() {
+        return (this.args.hash as Record<string, unknown>).name;
+      }
     }
 
-    this.registerComponent('Glimmer', 'FooBar', `{{yield @hash}}`, FooBar);
+    this.registerComponent('Glimmer', 'FooBar', `{{yield @hash this.alias}}`, FooBar);
 
-    this.render(`<FooBar @hash={{hash name=this.name}} as |values|>{{values.name}}</FooBar>`, {
-      name: 'Godfrey',
-    });
+    this.render(
+      `<FooBar @hash={{hash name=this.name}} as |values alias|>{{values.name}} {{alias}}</FooBar>`,
+      {
+        name: 'Godfrey',
+      }
+    );
 
-    this.assertHTML('Chad');
+    // JS alias will be updated, version accessed lazily in templates will not
+    this.assertHTML('Godfrey Chad');
     this.assertStableRerender();
 
     this.rerender({ name: 'Tom' });
 
-    this.assertHTML('Tom');
+    // Both will be updated to match the new value
+    this.assertHTML('Tom Tom');
     this.assertStableRerender();
 
-    assert.validateDeprecations(
-      /You set the 'name' property on a {{hash}} object. Setting properties on objects generated by {{hash}} is deprecated. Please update to use an object created with a tracked property or getter, or with a custom helper./
-    );
+    if (HAS_NATIVE_PROXY) {
+      assert.validateDeprecations(
+        /You set the 'name' property on a {{hash}} object. Setting properties on objects generated by {{hash}} is deprecated. Please update to use an object created with a tracked property or getter, or with a custom helper./
+      );
+    }
   }
 
   @test

diff --git a/packages/@glimmer/reference/lib/reference.ts b/packages/@glimmer/reference/lib/reference.ts
@@ -25,6 +25,7 @@ const enum ReferenceType {
 export interface Reference<_T = unknown> {
   [REFERENCE]: ReferenceType;
   debugLabel?: string;
+  children: null | Map<string | Reference, Reference>;
 }
 
 export default Reference;

diff --git a/packages/@glimmer/runtime/index.ts b/packages/@glimmer/runtime/index.ts
@@ -63,7 +63,7 @@ export {
 export { invokeHelper } from './lib/helpers/invoke';
 
 export { default as fn } from './lib/helpers/fn';
-export { default as hash, isHashProxy } from './lib/helpers/hash';
+export { default as hash } from './lib/helpers/hash';
 export { default as array } from './lib/helpers/array';
 export { default as get } from './lib/helpers/get';
 export { default as concat } from './lib/helpers/concat';

diff --git a/packages/@glimmer/runtime/lib/helpers/hash.ts b/packages/@glimmer/runtime/lib/helpers/hash.ts
@@ -1,146 +1,30 @@
-import { CapturedArguments, CapturedNamedArguments, Dict } from '@glimmer/interfaces';
-import { setCustomTagFor } from '@glimmer/manager';
-import { createComputeRef, createConstRef, Reference, valueForRef } from '@glimmer/reference';
-import { dict, HAS_NATIVE_PROXY, _WeakSet } from '@glimmer/util';
-import { combine, Tag, tagFor, track } from '@glimmer/validator';
+import { CapturedArguments, Dict } from '@glimmer/interfaces';
+import { createComputeRef, Reference } from '@glimmer/reference';
+import { reifyNamed } from '@glimmer/runtime';
 import { deprecate } from '@glimmer/global-context';
+import { HAS_NATIVE_PROXY } from '@glimmer/util';
 import { internalHelper } from './internal-helper';
-
-const HASH_PROXIES = new _WeakSet();
-
-export function isHashProxy(obj: unknown): boolean {
-  return HASH_PROXIES.has(obj as object);
-}
-
-function tagForKey(namedArgs: CapturedNamedArguments, key: string): Tag {
-  return track(() => {
-    if (key in namedArgs) {
-      valueForRef(namedArgs[key]);
-    }
-  });
-}
-
-let hashProxyFor: (args: CapturedNamedArguments) => Record<string, unknown>;
-
-class HashProxy implements ProxyHandler<Record<string, unknown>> {
-  constructor(private named: CapturedNamedArguments, private target: Record<string, unknown>) {}
-
-  private argsCaches = dict<Reference>();
-
-  syncKey(key: string | number) {
-    let { argsCaches, named } = this;
-
-    if (!(key in named)) return;
-
-    let cache = argsCaches[key];
-
-    if (cache === undefined) {
-      const ref = this.named[key as string];
-
-      argsCaches[key] = cache = createComputeRef(() => {
-        this.target[key] = valueForRef(ref);
-      });
-    }
-
-    valueForRef(cache);
-  }
-
-  get(target: Record<string, unknown>, prop: string | number) {
-    this.syncKey(prop);
-
-    return target[prop];
-  }
-
-  set(target: Record<string, unknown>, prop: string | number, value: unknown) {
-    deprecate(
-      `You set the '${prop}' property on a {{hash}} object. Setting properties on objects generated by {{hash}} is deprecated. Please update to use an object created with a tracked property or getter, or with a custom helper.`,
-      false,
-      { id: 'setting-on-hash' }
-    );
-
-    this.syncKey(prop);
-
-    target[prop] = value;
-
-    return true;
-  }
-
-  has(target: Record<string, unknown>, prop: string | number) {
-    return prop in this.named || prop in target;
-  }
-
-  ownKeys(target: {}) {
-    for (let key in this.named) {
-      this.syncKey(key);
-    }
-
-    return Object.getOwnPropertyNames(target);
-  }
-
-  getOwnPropertyDescriptor(target: {}, prop: string | number) {
-    if (prop in this.named) {
-      return {
-        enumerable: true,
-        configurable: true,
-        writable: true,
-      };
-    }
-
-    return Object.getOwnPropertyDescriptor(target, prop);
-  }
-}
-
-if (HAS_NATIVE_PROXY) {
-  hashProxyFor = (named) => {
-    const target = dict();
-    const proxy = new Proxy(target, new HashProxy(named, target));
-
-    setCustomTagFor(proxy, (_obj: object, key: string) => {
-      let argTag = tagForKey(named, key);
-      let proxyTag = tagFor(proxy, key);
-
-      return combine([argTag, proxyTag]);
+import { DEBUG } from '@glimmer/env';
+
+let wrapHashProxy: (hash: Record<string, unknown>) => Record<string, unknown>;
+
+if (DEBUG) {
+  wrapHashProxy = (hash: Record<string, unknown>) => {
+    return new Proxy(hash, {
+      set(target, key, value) {
+        deprecate(
+          `You set the '${String(
+            key
+          )}' property on a {{hash}} object. Setting properties on objects generated by {{hash}} is deprecated. Please update to use an object created with a tracked property or getter, or with a custom helper.`,
+          false,
+          { id: 'setting-on-hash' }
+        );
+
+        target[key as string] = value;
+
+        return true;
+      },
     });
-
-    HASH_PROXIES.add(proxy);
-
-    return proxy;
-  };
-} else {
-  hashProxyFor = (named) => {
-    const proxy = dict();
-
-    // Create a HashProxy handler to store the local state in case anyone
-    // overrides a named value. It handles all of the details in terms of
-    // syncing state up and returning the correct value based on autotracking.
-    const localState = dict();
-    const proxyHandler = new HashProxy(named, localState);
-
-    Object.keys(named).forEach((name) => {
-      Object.defineProperty(proxy, name, {
-        enumerable: true,
-        configurable: true,
-
-        get() {
-          return proxyHandler.get(localState, name);
-        },
-
-        set(value) {
-          return proxyHandler.set(localState, name, value);
-        },
-      });
-    });
-
-    setCustomTagFor(proxy, (_obj: object, key: string) => {
-      let argTag = tagForKey(named, key);
-      let proxyTag = tagFor(proxy, key);
-
-      return combine([argTag, proxyTag]);
-    });
-
-    HASH_PROXIES.add(proxy);
-
-    return proxy;
   };
 }
 
@@ -182,6 +66,30 @@ if (HAS_NATIVE_PROXY) {
  */
 export default internalHelper(
   ({ named }: CapturedArguments): Reference<Dict<unknown>> => {
-    return createConstRef(hashProxyFor(named), 'hash');
+    let ref = createComputeRef(
+      () => {
+        let hash = reifyNamed(named);
+
+        if (DEBUG && HAS_NATIVE_PROXY) {
+          hash = wrapHashProxy(hash);
+        }
+
+        return hash;
+      },
+      null,
+      'hash'
+    );
+
+    // Setup the children so that templates can bypass getting the value of
+    // the reference and treat children lazily
+    let children = new Map();
+
+    for (let name in named) {
+      children.set(name, named[name]);
+    }
+
+    ref.children = children;
+
+    return ref;
   }
 );