Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert hash to bi-modal behavior #1323

Merged
merged 1 commit into from
Jun 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 30 additions & 13 deletions packages/@glimmer/integration-tests/test/helpers/hash-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
}

Expand All @@ -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
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions packages/@glimmer/reference/lib/reference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion packages/@glimmer/runtime/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
190 changes: 49 additions & 141 deletions packages/@glimmer/runtime/lib/helpers/hash.ts
Original file line number Diff line number Diff line change
@@ -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;
};
}

Expand Down Expand Up @@ -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;
}
);