From 52f5257c6013c6dde92790618328238ed589ee1d Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Tue, 22 Jun 2021 16:25:49 -0700 Subject: [PATCH] Revert hash to bi-modal behavior This PR reverts hash to it's bi-modal behavior prior to Ember ~3.22. Hash will now: 1. Eagerly produce an object whenever used in JS, which consumes all references and autotracks them. Whenever one reference changes, the hash will dirty and produce a new object. 2. Lazily allow access to child paths in templates, e.g. `{{hash.foo}}` will _not_ access any other references on the hash. These behaviors have some quirks, but the quirks existed before 3.22 and were primarily related to the fact that the object produced in 1 is mutable and can diverge from the values in 2. Since setting values on a hash object is deprecated, this should not be an issue in the near future. --- .../test/helpers/hash-test.ts | 43 ++-- packages/@glimmer/reference/lib/reference.ts | 1 + packages/@glimmer/runtime/index.ts | 2 +- packages/@glimmer/runtime/lib/helpers/hash.ts | 190 +++++------------- 4 files changed, 81 insertions(+), 155 deletions(-) diff --git a/packages/@glimmer/integration-tests/test/helpers/hash-test.ts b/packages/@glimmer/integration-tests/test/helpers/hash-test.ts index 2a82014275..7a3c227d7e 100644 --- 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 }) { 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 { `{{values.firstName}} {{values.lastName}}` ); - 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).name; + } } - this.registerComponent('Glimmer', 'FooBar', `{{yield @hash}}`, FooBar); + this.registerComponent('Glimmer', 'FooBar', `{{yield @hash this.alias}}`, FooBar); - this.render(`{{values.name}}`, { - name: 'Godfrey', - }); + this.render( + `{{values.name}} {{alias}}`, + { + 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 index 39ec29123b..cbd224b5a0 100644 --- 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; } export default Reference; diff --git a/packages/@glimmer/runtime/index.ts b/packages/@glimmer/runtime/index.ts index a142b6875c..0bac53dfd8 100644 --- 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 index d4908d410e..51ce8249bd 100644 --- 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; - -class HashProxy implements ProxyHandler> { - constructor(private named: CapturedNamedArguments, private target: Record) {} - - private argsCaches = dict(); - - 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, prop: string | number) { - this.syncKey(prop); - - return target[prop]; - } - - set(target: Record, 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, 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) => Record; + +if (DEBUG) { + wrapHashProxy = (hash: Record) => { + 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> => { - 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; } );