[BUGFIX] Make the custom tag system non-enumerable

packages/@glimmer/manager/index.ts

@@ -12,4 +12,4 @@ export { modifierCapabilities, CustomModifierManager } from './lib/public/modifi
 export { helperCapabilities, hasDestroyable, hasValue, customHelper } from './lib/public/helper';
 export { getComponentTemplate, setComponentTemplate } from './lib/public/template';
 export { capabilityFlagsFrom, hasCapability, managerHasCapability } from './lib/util/capabilities';
-export { CUSTOM_TAG_FOR } from './lib/util/args-proxy';
+export { getCustomTagFor, setCustomTagFor } from './lib/util/args-proxy';

packages/@glimmer/manager/lib/util/args-proxy.ts

@@ -6,10 +6,18 @@ import {
   CapturedPositionalArguments,
 } from '@glimmer/interfaces';
 import { Reference, valueForRef } from '@glimmer/reference';
-import { HAS_NATIVE_PROXY, enumerableSymbol } from '@glimmer/util';
+import { HAS_NATIVE_PROXY } from '@glimmer/util';
 import { Tag, track } from '@glimmer/validator';
 
-export const CUSTOM_TAG_FOR = enumerableSymbol('CUSTOM_TAG_FOR');
+const CUSTOM_TAG_FOR = new WeakMap<object, (obj: object, key: string) => Tag>();
+
+export function getCustomTagFor(obj: object): ((obj: object, key: string) => Tag) | undefined {
+  return CUSTOM_TAG_FOR.get(obj);
+}
+
+export function setCustomTagFor(obj: object, customTagFn: (obj: object, key: string) => Tag) {
+  CUSTOM_TAG_FOR.set(obj, customTagFn);
+}
 
 function convertToInt(prop: number | string | symbol): number | null {
   if (typeof prop === 'symbol') return null;
@@ -50,81 +58,89 @@ export let argsProxyFor: (
   type: 'component' | 'helper' | 'modifier'
 ) => Arguments;
 
-if (HAS_NATIVE_PROXY) {
-  argsProxyFor = (capturedArgs, type) => {
-    const { named, positional } = capturedArgs;
+class NamedArgsProxy implements ProxyHandler<{}> {
+  declare set?: (target: {}, prop: string | number | symbol) => boolean;
 
-    let getNamedTag = (key: string) => tagForNamedArg(named, key);
-    let getPositionalTag = (key: string) => tagForPositionalArg(positional, key);
+  constructor(private named: CapturedNamedArguments) {}
 
-    const namedHandler: ProxyHandler<{}> = {
-      get(_target, prop) {
-        const ref = named[prop as string];
+  get(_target: {}, prop: string | number | symbol) {
+    const ref = this.named[prop as string];
 
-        if (ref !== undefined) {
-          return valueForRef(ref);
-        } else if (prop === CUSTOM_TAG_FOR) {
-          return getNamedTag;
-        }
-      },
-
-      has(_target, prop) {
-        return prop in named;
-      },
-
-      ownKeys(_target) {
-        return Object.keys(named);
-      },
-
-      isExtensible() {
-        return false;
-      },
-
-      getOwnPropertyDescriptor(_target, prop) {
-        if (DEBUG && !(prop in named)) {
-          throw new Error(
-            `args proxies do not have real property descriptors, so you should never need to call getOwnPropertyDescriptor yourself. This code exists for enumerability, such as in for-in loops and Object.keys(). Attempted to get the descriptor for \`${String(
-              prop
-            )}\``
-          );
-        }
-
-        return {
-          enumerable: true,
-          configurable: true,
-        };
-      },
+    if (ref !== undefined) {
+      return valueForRef(ref);
+    }
+  }
+
+  has(_target: {}, prop: string | number | symbol) {
+    return prop in this.named;
+  }
+
+  ownKeys() {
+    return Object.keys(this.named);
+  }
+
+  isExtensible() {
+    return false;
+  }
+
+  getOwnPropertyDescriptor(_target: {}, prop: string | number | symbol) {
+    if (DEBUG && !(prop in this.named)) {
+      throw new Error(
+        `args proxies do not have real property descriptors, so you should never need to call getOwnPropertyDescriptor yourself. This code exists for enumerability, such as in for-in loops and Object.keys(). Attempted to get the descriptor for \`${String(
+          prop
+        )}\``
+      );
+    }
+
+    return {
+      enumerable: true,
+      configurable: true,
     };
+  }
+}
 
-    const positionalHandler: ProxyHandler<[]> = {
-      get(target, prop) {
-        if (prop === 'length') {
-          return positional.length;
-        }
+class PositionalArgsProxy implements ProxyHandler<[]> {
+  declare set?: (target: [], prop: string | number | symbol) => boolean;
+  declare ownKeys?: (target: []) => string[];
 
-        const parsed = convertToInt(prop);
+  constructor(private positional: CapturedPositionalArguments) {}
 
-        if (parsed !== null && parsed < positional.length) {
-          return valueForRef(positional[parsed]);
-        }
+  get(target: [], prop: string | number | symbol) {
+    let { positional } = this;
 
-        if (prop === CUSTOM_TAG_FOR) {
-          return getPositionalTag;
-        }
+    if (prop === 'length') {
+      return positional.length;
+    }
 
-        return (target as any)[prop];
-      },
+    const parsed = convertToInt(prop);
 
-      isExtensible() {
-        return false;
-      },
+    if (parsed !== null && parsed < positional.length) {
+      return valueForRef(positional[parsed]);
+    }
 
-      has(_target, prop) {
-        const parsed = convertToInt(prop);
+    return (target as any)[prop];
+  }
 
-        return parsed !== null && parsed < positional.length;
-      },
-    };
+  isExtensible() {
+    return false;
+  }
+
+  has(_target: [], prop: string | number | symbol) {
+    const parsed = convertToInt(prop);
+
+    return parsed !== null && parsed < this.positional.length;
+  }
+}
+
+if (HAS_NATIVE_PROXY) {
+  argsProxyFor = (capturedArgs, type) => {
+    const { named, positional } = capturedArgs;
+
+    let getNamedTag = (_obj: object, key: string) => tagForNamedArg(named, key);
+    let getPositionalTag = (_obj: object, key: string) => tagForPositionalArg(positional, key);
+
+    const namedHandler = new NamedArgsProxy(named);
+    const positionalHandler = new PositionalArgsProxy(positional);
 
     const namedTarget = Object.create(null);
     const positionalTarget: unknown[] = [];
@@ -149,25 +165,29 @@ if (HAS_NATIVE_PROXY) {
       positionalHandler.ownKeys = forInDebugHandler;
     }
 
+    const namedProxy = new Proxy(namedTarget, namedHandler);
+    const positionalProxy = new Proxy(positionalTarget, positionalHandler);
+
+    setCustomTagFor(namedProxy, getNamedTag);
+    setCustomTagFor(positionalProxy, getPositionalTag);
+
     return {
-      named: new Proxy(namedTarget, namedHandler),
-      positional: new Proxy(positionalTarget, positionalHandler),
+      named: namedProxy,
+      positional: positionalProxy,
     };
   };
 } else {
   argsProxyFor = (capturedArgs, _type) => {
     const { named, positional } = capturedArgs;
 
-    let getNamedTag = (key: string) => tagForNamedArg(named, key);
-    let getPositionalTag = (key: string) => tagForPositionalArg(positional, key);
+    let getNamedTag = (_obj: object, key: string) => tagForNamedArg(named, key);
+    let getPositionalTag = (_obj: object, key: string) => tagForPositionalArg(positional, key);
 
     let namedProxy = {};
+    let positionalProxy: unknown[] = [];
 
-    Object.defineProperty(namedProxy, CUSTOM_TAG_FOR, {
-      configurable: false,
-      enumerable: false,
-      value: getNamedTag,
-    });
+    setCustomTagFor(namedProxy, getNamedTag);
+    setCustomTagFor(positionalProxy, getPositionalTag);
 
     Object.keys(named).forEach((name) => {
       Object.defineProperty(namedProxy, name, {
@@ -179,14 +199,6 @@ if (HAS_NATIVE_PROXY) {
       });
     });
 
-    let positionalProxy: unknown[] = [];
-
-    Object.defineProperty(positionalProxy, CUSTOM_TAG_FOR, {
-      configurable: false,
-      enumerable: false,
-      value: getPositionalTag,
-    });
-
     positional.forEach((ref: Reference, index: number) => {
       Object.defineProperty(positionalProxy, index, {
         enumerable: true,

packages/@glimmer/util/index.ts

@@ -23,6 +23,7 @@ export * from './lib/template';
 export { default as _WeakSet } from './lib/weak-set';
 export { castToSimple, castToBrowser, checkNode } from './lib/simple-cast';
 export * from './lib/present';
+export { default as intern } from './lib/intern';
 
 export { default as debugToString } from './lib/debug-to-string';
 export { beginTestSteps, endTestSteps, logStep, verifySteps } from './lib/debug-steps';

packages/@glimmer/util/lib/intern.ts

@@ -0,0 +1,49 @@
+/**
+  Strongly hint runtimes to intern the provided string.
+
+  When do I need to use this function?
+
+  For the most part, never. Pre-mature optimization is bad, and often the
+  runtime does exactly what you need it to, and more often the trade-off isn't
+  worth it.
+
+  Why?
+
+  Runtimes store strings in at least 2 different representations:
+  Ropes and Symbols (interned strings). The Rope provides a memory efficient
+  data-structure for strings created from concatenation or some other string
+  manipulation like splitting.
+
+  Unfortunately checking equality of different ropes can be quite costly as
+  runtimes must resort to clever string comparison algorithms. These
+  algorithms typically cost in proportion to the length of the string.
+  Luckily, this is where the Symbols (interned strings) shine. As Symbols are
+  unique by their string content, equality checks can be done by pointer
+  comparison.
+
+  How do I know if my string is a rope or symbol?
+
+  Typically (warning general sweeping statement, but truthy in runtimes at
+  present) static strings created as part of the JS source are interned.
+  Strings often used for comparisons can be interned at runtime if some
+  criteria are met.  One of these criteria can be the size of the entire rope.
+  For example, in chrome 38 a rope longer then 12 characters will not
+  intern, nor will segments of that rope.
+
+  Some numbers: http://jsperf.com/eval-vs-keys/8
+
+  Known Trick™
+
+  @private
+  @return {String} interned version of the provided string
+*/
+export default function intern(str: string): string {
+  let obj: Record<string, number> = {};
+  obj[str] = 1;
+  for (let key in obj) {
+    if (key === str) {
+      return key;
+    }
+  }
+  return str;
+}

packages/@glimmer/util/lib/platform-utils.ts

@@ -1,4 +1,5 @@
 import { Maybe } from '@glimmer/interfaces';
+import intern from './intern';
 
 export type Factory<T> = new (...args: unknown[]) => T;
 
@@ -40,7 +41,7 @@ export type Lit = string | number | boolean | undefined | null | void | {};
 export const tuple = <T extends Lit[]>(...args: T) => args;
 
 export function enumerableSymbol(key: string): any {
-  return `__${key}${Math.floor(Math.random() * Date.now())}__`;
+  return intern(`__${key}${Math.floor(Math.random() * Date.now())}__`);
 }
 
 export const symbol = HAS_NATIVE_SYMBOL ? Symbol : enumerableSymbol;