Allow all (hash) keys to be set

packages/@glimmer/integration-tests/test/helpers/hash-test.ts

@@ -1,4 +1,5 @@
 import { jitSuite, RenderTest, test, GlimmerishComponent, tracked } from '../..';
+import { HAS_NATIVE_PROXY } from '@glimmer/util';
 
 class HashTest extends RenderTest {
   static suiteName = 'Helpers test: {{hash}}';
@@ -183,7 +184,7 @@ class HashTest extends RenderTest {
   }
 
   @test
-  'defined hash keys cannot be updated'(assert: Assert) {
+  'defined hash keys can be updated'(assert: Assert) {
     class FooBar extends GlimmerishComponent {
       constructor(owner: object, args: { hash: Record<string, unknown> }) {
         super(owner, args);
@@ -193,17 +194,48 @@ class HashTest extends RenderTest {
 
     this.registerComponent('Glimmer', 'FooBar', `{{yield @hash}}`, FooBar);
 
-    assert.throws(() => {
-      this.render(`
-        <FooBar @hash={{hash firstName="Godfrey"}} as |values|>
-          {{values.firstName}} {{values.lastName}}
-        </FooBar>
-      `);
-    }, /You attempted to set the "firstName" value on an object generated using the \(hash\) helper|Assignment to read-only properties is not allowed in strict mode/);
+    this.render(
+      `<FooBar @hash={{hash firstName="Godfrey" lastName="Hietala"}} as |values|>{{values.firstName}} {{values.lastName}}</FooBar>`
+    );
+
+    this.assertHTML('Chad 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./
+    );
+  }
+
+  @test
+  'defined hash keys are reset whenever the upstream changes'(assert: Assert) {
+    class FooBar extends GlimmerishComponent {
+      constructor(owner: object, args: { hash: Record<string, unknown> }) {
+        super(owner, args);
+        args.hash.name = 'Chad';
+      }
+    }
+
+    this.registerComponent('Glimmer', 'FooBar', `{{yield @hash}}`, FooBar);
+
+    this.render(`<FooBar @hash={{hash name=this.name}} as |values|>{{values.name}}</FooBar>`, {
+      name: 'Godfrey',
+    });
+
+    this.assertHTML('Chad');
+    this.assertStableRerender();
+
+    this.rerender({ name: 'Tom' });
+
+    this.assertHTML('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./
+    );
   }
 
   @test
-  'undefined hash keys can be updated'() {
+  'undefined hash keys can be updated'(assert: Assert) {
     class FooBar extends GlimmerishComponent {
       constructor(owner: object, args: { hash: Record<string, unknown> }) {
         super(owner, args);
@@ -219,6 +251,12 @@ class HashTest extends RenderTest {
 
     this.assertHTML('Godfrey Chan');
     this.assertStableRerender();
+
+    if (HAS_NATIVE_PROXY) {
+      assert.validateDeprecations(
+        /You set the 'lastName' 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./
+      );
+    }
   }
 }
 

packages/@glimmer/runtime/lib/helpers/hash.ts

@@ -1,82 +1,130 @@
 import { CapturedArguments, CapturedNamedArguments, Dict } from '@glimmer/interfaces';
 import { setCustomTagFor } from '@glimmer/manager';
-import { createConstRef, Reference, valueForRef } from '@glimmer/reference';
+import { createComputeRef, createConstRef, Reference, valueForRef } from '@glimmer/reference';
 import { dict, HAS_NATIVE_PROXY } from '@glimmer/util';
-import { Tag, track } from '@glimmer/validator';
-import { assert } from '@glimmer/global-context';
+import { combine, Tag, tagFor, track } from '@glimmer/validator';
+import { deprecate } from '@glimmer/global-context';
 import { internalHelper } from './internal-helper';
 
-function tagForKey(hash: Record<string, unknown>, key: string): Tag {
-  return track(() => hash[key]);
+function tagForKey(hash: CapturedNamedArguments, key: string): Tag {
+  return track(() => valueForRef(hash[key]));
 }
 
 let hashProxyFor: (args: CapturedNamedArguments) => Record<string, unknown>;
 
 class HashProxy implements ProxyHandler<Record<string, unknown>> {
-  constructor(private named: CapturedNamedArguments) {}
+  constructor(private named: CapturedNamedArguments, private target: Record<string, unknown>) {}
 
-  get(target: Record<string, unknown>, prop: string | number, receiver: unknown) {
-    const ref = this.named[prop as string];
+  private argsCaches = dict<Reference>();
 
-    if (ref !== undefined) {
-      return valueForRef(ref);
-    } else {
-      return Reflect.get(target, prop, receiver);
+  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);
   }
 
-  set(target: Record<string, unknown>, prop: string | number, receiver: unknown) {
-    assert(
-      !(prop in this.named),
-      `You attempted to set the "${prop}" value on an object generated using the (hash) helper, but this is not supported because it was defined on the original hash and is a reference to the original value. You can set values which were _not_ defined on the hash, but you cannot set values defined on the original hash (e.g. {{hash myValue=123}})`
+  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' }
     );
 
-    return Reflect.set(target, prop, receiver);
+    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: {}) {
-    return Reflect.ownKeys(this.named).concat(Reflect.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 Reflect.getOwnPropertyDescriptor(target, prop);
+    return Object.getOwnPropertyDescriptor(target, prop);
   }
 }
 
 if (HAS_NATIVE_PROXY) {
   hashProxyFor = (named) => {
-    const proxy = new Proxy(dict(), new HashProxy(named));
+    const target = dict();
+    const proxy = new Proxy(target, new HashProxy(named, target));
 
-    setCustomTagFor(proxy, (_obj: object, key: string) => tagForKey(named, key));
+    setCustomTagFor(proxy, (_obj: object, key: string) => {
+      let argTag = tagForKey(named, key);
+      let proxyTag = tagFor(proxy, key);
+
+      return combine([argTag, proxyTag]);
+    });
 
     return proxy;
   };
 } else {
   hashProxyFor = (named) => {
-    let proxy = dict();
+    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 valueForRef(named[name]);
+          return proxyHandler.get(localState, name);
+        },
+
+        set(value) {
+          return proxyHandler.set(localState, name, value);
         },
       });
     });
 
-    setCustomTagFor(proxy, (_obj: object, key: string) => tagForKey(named, key));
+    setCustomTagFor(proxy, (_obj: object, key: string) => {
+      let argTag = tagForKey(named, key);
+      let proxyTag = tagFor(proxy, key);
+
+      return combine([argTag, proxyTag]);
+    });
 
     return proxy;
   };