Address some of Godfrey's comments

glimmerjs · Jun 21, 2017 · b58d4d7 · b58d4d7
1 parent c05bc54
commit b58d4d7
Show file tree

Hide file tree

Showing 6 changed files with 11 additions and 35 deletions.
diff --git a/packages/@glimmer/runtime/lib/dom/helper.ts b/packages/@glimmer/runtime/lib/dom/helper.ts
@@ -125,7 +125,7 @@ export namespace DOM {
       return this.document.createElementNS(namespace, tag);
     }
 
-    setAttribute(element: Element, name: string, value: string, namespace?: Option<string>) {
+    setAttribute(element: Element, name: string, value: string, namespace: Option<string> = null) {
       if (namespace) {
         element.setAttributeNS(namespace, name, value);
       } else {

diff --git a/packages/@glimmer/runtime/lib/dom/normalize.ts b/packages/@glimmer/runtime/lib/dom/normalize.ts
@@ -31,19 +31,6 @@ export function normalizeTrustedValue(value: Opaque): TrustingInsertion {
   return String(value);
 }
 
-export function normalizeValue(value: Opaque): CautiousInsertion {
-  if (isEmpty(value)) {
-    return '';
-  }
-  if (isString(value)) {
-    return value;
-  }
-  if (isSafeString(value) || isNode(value)) {
-    return value;
-  }
-  return String(value);
-}
-
 export function isEmpty(value: Opaque): boolean {
   return value === null || value === undefined || typeof value.toString !== 'function';
 }

diff --git a/packages/@glimmer/runtime/lib/environment.ts b/packages/@glimmer/runtime/lib/environment.ts
@@ -8,10 +8,6 @@ import { DOMChanges, DOMTreeConstruction } from './dom/helper';
 import { Reference, OpaqueIterable } from '@glimmer/reference';
 import { UNDEFINED_REFERENCE, ConditionalReference } from './references';
 import { DynamicAttributeFactory, defaultDynamicAttributes } from './vm/attributes/dynamic';
-// import {
-//   defaultManagers,
-//   AttributeManager
-// } from './dom/attribute-managers';
 
 import {
   PartialDefinition
@@ -456,7 +452,7 @@ export abstract class Environment {
     transaction.commit();
   }
 
-  attributeFor(element: Simple.Element, attr: string, isTrusting: boolean, namespace: Option<string>): DynamicAttributeFactory {
+  attributeFor(element: Simple.Element, attr: string, isTrusting: boolean, namespace: Option<string> = null): DynamicAttributeFactory {
     return defaultDynamicAttributes(element, attr);
   }
 

diff --git a/packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts b/packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts
@@ -5,6 +5,7 @@ import { sanitizeAttributeValue, requiresSanitization } from '../../dom/sanitize
 import { normalizeProperty } from '../../dom/props';
 import { SVG_NAMESPACE } from '../../dom/helper';
 import { Attribute, AttributeOperation } from './index';
+import { normalizeStringValue } from "@glimmer/runtime/lib/dom/normalize";
 
 export interface DynamicAttributeFactory {
   new(attribute: Attribute): DynamicAttribute;
@@ -59,7 +60,7 @@ export abstract class DynamicAttribute implements AttributeOperation {
 
 export class SimpleDynamicAttribute extends DynamicAttribute {
   set(dom: ElementBuilder, value: Opaque): void {
-    let normalizedValue = normalizeStringValue(value);
+    let normalizedValue = normalizeValue(value);
 
     if (normalizedValue !== null) {
       let { name, namespace } = this.attribute;
@@ -68,7 +69,7 @@ export class SimpleDynamicAttribute extends DynamicAttribute {
   }
 
   update(value: Opaque): void {
-    let normalizedValue = normalizeStringValue(value);
+    let normalizedValue = normalizeValue(value);
     let { element, name } = this.attribute;
 
     if (normalizedValue === null) {
@@ -126,13 +127,13 @@ export class SafeDynamicProperty extends DefaultDynamicProperty {
 
 export class InputValueDynamicAttribute extends DefaultDynamicProperty {
   set(dom: ElementBuilder, value: Opaque) {
-    dom.__setProperty('value', normalizeInputValue(value));
+    dom.__setProperty('value', normalizeStringValue(value));
   }
 
   update(value: Opaque) {
     let input = <HTMLInputElement>this.attribute.element;
     let currentValue = input.value;
-    let normalizedValue = normalizeInputValue(value);
+    let normalizedValue = normalizeStringValue(value);
     if (currentValue !== normalizedValue) {
       input.value = normalizedValue!;
     }
@@ -165,15 +166,7 @@ function isUserInputValue(tagName: string, attribute: string) {
   return (tagName === 'INPUT' || tagName === 'TEXTAREA') && attribute === 'value';
 }
 
-function normalizeInputValue(value: Opaque) {
-  if (value === null || value === undefined || typeof value.toString !== 'function') {
-    return '';
-  }
-
-  return `${value}`;
-}
-
-function normalizeStringValue(value: Opaque): Option<string> {
+function normalizeValue(value: Opaque): Option<string> {
   if (value === false || value === undefined || value === null || typeof value.toString === 'undefined') {
     return null;
   }

diff --git a/packages/@glimmer/runtime/lib/vm/content/html.ts b/packages/@glimmer/runtime/lib/vm/content/html.ts
@@ -14,7 +14,7 @@ export default class DynamicHTMLContent extends DynamicContentBase {
 
     if (value === lastValue) return this;
 
-    if (isSafeString(value) && isSafeString(lastValue) && value.toHTML() === lastValue.toHTML()) {
+    if (isSafeString(value) && value.toHTML() === lastValue.toHTML()) {
       this.lastValue = value;
       return this;
     }
@@ -31,7 +31,7 @@ export class DynamicTrustedHTMLContent extends DynamicContentBase {
   update(env: Environment, value: Opaque): DynamicContent {
     let { lastValue } = this;
 
-    if (typeof value === 'string' && value === lastValue) return this;
+    if (value === lastValue) return this;
     let newValue = normalizeTrustedValue(value);
     if (newValue === lastValue) return this;
 

diff --git a/tsconfig.json b/tsconfig.json
@@ -16,7 +16,7 @@
     "noImplicitAny": true,
     "suppressImplicitAnyIndexErrors": true,
     "noUnusedLocals": true,
-    // "noUnusedParameters": true,
+    "noUnusedParameters": true,
     "allowUnreachableCode": false,
     "strictNullChecks": true,
     "noImplicitReturns": true,