From 2d8da74aeba3da453e05573340fc96a9eb109c0a Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Wed, 7 Sep 2016 13:12:09 -0700 Subject: [PATCH 1/6] Make internal helpers implement `Helper` interface Follow up for https://github.com/emberjs/ember.js/pull/14149#discussion_r77218933 --- packages/ember-glimmer/lib/environment.js | 18 +++--- packages/ember-glimmer/lib/helpers/-class.js | 9 +-- .../ember-glimmer/lib/helpers/-html-safe.js | 9 +-- .../ember-glimmer/lib/helpers/-input-type.js | 9 +-- .../lib/helpers/-normalize-class.js | 9 +-- packages/ember-glimmer/lib/helpers/action.js | 10 +--- .../ember-glimmer/lib/helpers/component.js | 10 +--- packages/ember-glimmer/lib/helpers/concat.js | 10 ++-- packages/ember-glimmer/lib/helpers/each-in.js | 13 ++--- packages/ember-glimmer/lib/helpers/get.js | 10 +--- packages/ember-glimmer/lib/helpers/hash.js | 9 +-- .../ember-glimmer/lib/helpers/if-unless.js | 46 +++++++-------- packages/ember-glimmer/lib/helpers/loc.js | 10 ++-- packages/ember-glimmer/lib/helpers/log.js | 10 ++-- packages/ember-glimmer/lib/helpers/mut.js | 58 +++++++++---------- .../ember-glimmer/lib/helpers/query-param.js | 9 +-- .../ember-glimmer/lib/helpers/readonly.js | 16 ++--- packages/ember-glimmer/lib/helpers/unbound.js | 19 +++--- 18 files changed, 120 insertions(+), 164 deletions(-) diff --git a/packages/ember-glimmer/lib/environment.js b/packages/ember-glimmer/lib/environment.js index 8acadb68df7..ede1f857bf4 100644 --- a/packages/ember-glimmer/lib/environment.js +++ b/packages/ember-glimmer/lib/environment.js @@ -121,7 +121,7 @@ export default class Environment extends GlimmerEnvironment { '-input-type': inputTypeHelper, '-normalize-class': normalizeClassHelper, '-html-safe': htmlSafeHelper, - '-get-dynamic-var': { glimmerNativeHelper: getDynamicVar } + '-get-dynamic-var': getDynamicVar }; } @@ -288,23 +288,23 @@ export default class Environment extends GlimmerEnvironment { assert('The first argument passed into `lookupHelper` should be an array', Array.isArray(nameParts)); let name = nameParts[0]; + let helper = this.builtInHelpers[name]; + + if (helper) { + return helper; + } + let blockMeta = symbolTable.getMeta(); let owner = blockMeta.owner; let options = blockMeta.moduleName && { source: `template:${blockMeta.moduleName}` } || {}; - let helper = this.builtInHelpers[name] || - owner.lookup(`helper:${name}`, options) || - owner.lookup(`helper:${name}`); + helper = owner.lookup(`helper:${name}`, options) || owner.lookup(`helper:${name}`); // TODO: try to unify this into a consistent protocol to avoid wasteful closure allocations - if (helper.isInternalHelper) { - return (vm, args) => helper.toReference(args, this, symbolTable); - } else if (helper.isHelperInstance) { + if (helper.isHelperInstance) { return (vm, args) => SimpleHelperReference.create(helper.compute, args); } else if (helper.isHelperFactory) { return (vm, args) => ClassBasedHelperReference.create(helper, vm, args); - } else if (helper.glimmerNativeHelper) { - return helper.glimmerNativeHelper; } else { throw new Error(`${nameParts} is not a helper`); } diff --git a/packages/ember-glimmer/lib/helpers/-class.js b/packages/ember-glimmer/lib/helpers/-class.js index db13930a835..f5709df6f01 100644 --- a/packages/ember-glimmer/lib/helpers/-class.js +++ b/packages/ember-glimmer/lib/helpers/-class.js @@ -23,9 +23,6 @@ function classHelper({ positional }) { return value; } -export default { - isInternalHelper: true, - toReference(args) { - return new InternalHelperReference(classHelper, args); - } -}; +export default function(vm, args) { + return new InternalHelperReference(classHelper, args); +} diff --git a/packages/ember-glimmer/lib/helpers/-html-safe.js b/packages/ember-glimmer/lib/helpers/-html-safe.js index 70f7fb84440..72f538c5037 100644 --- a/packages/ember-glimmer/lib/helpers/-html-safe.js +++ b/packages/ember-glimmer/lib/helpers/-html-safe.js @@ -6,9 +6,6 @@ function htmlSafe({ positional }) { return new SafeString(path.value()); } -export default { - isInternalHelper: true, - toReference(args) { - return new InternalHelperReference(htmlSafe, args); - } -}; +export default function(vm, args) { + return new InternalHelperReference(htmlSafe, args); +} diff --git a/packages/ember-glimmer/lib/helpers/-input-type.js b/packages/ember-glimmer/lib/helpers/-input-type.js index 111afb7cb21..a51d2fbeb6a 100644 --- a/packages/ember-glimmer/lib/helpers/-input-type.js +++ b/packages/ember-glimmer/lib/helpers/-input-type.js @@ -8,9 +8,6 @@ function inputTypeHelper({ positional, named }) { return '-text-field'; } -export default { - isInternalHelper: true, - toReference(args) { - return new InternalHelperReference(inputTypeHelper, args); - } -}; +export default function(vm, args) { + return new InternalHelperReference(inputTypeHelper, args); +} diff --git a/packages/ember-glimmer/lib/helpers/-normalize-class.js b/packages/ember-glimmer/lib/helpers/-normalize-class.js index a916e86e823..c53b3d3293f 100644 --- a/packages/ember-glimmer/lib/helpers/-normalize-class.js +++ b/packages/ember-glimmer/lib/helpers/-normalize-class.js @@ -15,9 +15,6 @@ function normalizeClass({ positional, named }) { } } -export default { - isInternalHelper: true, - toReference(args) { - return new InternalHelperReference(normalizeClass, args); - } -}; +export default function(vm, args) { + return new InternalHelperReference(normalizeClass, args); +} diff --git a/packages/ember-glimmer/lib/helpers/action.js b/packages/ember-glimmer/lib/helpers/action.js index b1fc0d4e8e8..3fb14da767f 100644 --- a/packages/ember-glimmer/lib/helpers/action.js +++ b/packages/ember-glimmer/lib/helpers/action.js @@ -81,13 +81,9 @@ export class ClosureActionReference extends CachedReference { } } -export default { - isInternalHelper: true, - - toReference(args) { - return ClosureActionReference.create(args); - } -}; +export default function(vm, args) { + return ClosureActionReference.create(args); +} export function createClosureAction(target, action, valuePath, actionArgs) { let closureAction; diff --git a/packages/ember-glimmer/lib/helpers/component.js b/packages/ember-glimmer/lib/helpers/component.js index ef0d608941a..2ab0dd17a80 100644 --- a/packages/ember-glimmer/lib/helpers/component.js +++ b/packages/ember-glimmer/lib/helpers/component.js @@ -122,10 +122,6 @@ function curryArgs(definition, newArgs) { return mergedArgs; } -export default { - isInternalHelper: true, - - toReference(args, env, symbolTable) { - return ClosureComponentReference.create(args, symbolTable, env); - } -}; +export default function(vm, args, symbolTable) { + return ClosureComponentReference.create(args, symbolTable, vm.env); +} diff --git a/packages/ember-glimmer/lib/helpers/concat.js b/packages/ember-glimmer/lib/helpers/concat.js index 443a145ca9b..f6a8e7a38dd 100644 --- a/packages/ember-glimmer/lib/helpers/concat.js +++ b/packages/ember-glimmer/lib/helpers/concat.js @@ -1,4 +1,4 @@ -import { helper } from '../helper'; +import { InternalHelperReference } from '../utils/references'; import { normalizeTextValue } from 'glimmer-runtime'; /** @@ -22,8 +22,10 @@ import { normalizeTextValue } from 'glimmer-runtime'; @for Ember.Templates.helpers @since 1.13.0 */ -function concat(args) { - return args.map(normalizeTextValue).join(''); +function concat({ positional }) { + return positional.value().map(normalizeTextValue).join(''); } -export default helper(concat); +export default function(vm, args) { + return new InternalHelperReference(concat, args); +} diff --git a/packages/ember-glimmer/lib/helpers/each-in.js b/packages/ember-glimmer/lib/helpers/each-in.js index c830e8f9965..f12f3ac9f5b 100644 --- a/packages/ember-glimmer/lib/helpers/each-in.js +++ b/packages/ember-glimmer/lib/helpers/each-in.js @@ -41,11 +41,8 @@ export function isEachIn(ref) { return ref && ref[EACH_IN_REFERENCE]; } -export default { - isInternalHelper: true, - toReference(args) { - let ref = Object.create(args.positional.at(0)); - ref[EACH_IN_REFERENCE] = true; - return ref; - } -}; +export default function(vm, args) { + let ref = Object.create(args.positional.at(0)); + ref[EACH_IN_REFERENCE] = true; + return ref; +} diff --git a/packages/ember-glimmer/lib/helpers/get.js b/packages/ember-glimmer/lib/helpers/get.js index 8377794ceb1..25e58a26c90 100644 --- a/packages/ember-glimmer/lib/helpers/get.js +++ b/packages/ember-glimmer/lib/helpers/get.js @@ -51,13 +51,9 @@ import { CONSTANT_TAG, UpdatableTag, combine, isConst, referenceFromParts } from @since 2.1.0 */ -export default { - isInternalHelper: true, - - toReference(args) { - return GetHelperReference.create(args.positional.at(0), args.positional.at(1)); - } -}; +export default function(vm, args) { + return GetHelperReference.create(args.positional.at(0), args.positional.at(1)); +} class GetHelperReference extends CachedReference { static create(sourceReference, pathReference) { diff --git a/packages/ember-glimmer/lib/helpers/hash.js b/packages/ember-glimmer/lib/helpers/hash.js index ad960504697..4c062863464 100644 --- a/packages/ember-glimmer/lib/helpers/hash.js +++ b/packages/ember-glimmer/lib/helpers/hash.js @@ -30,9 +30,6 @@ @public */ -export default { - isInternalHelper: true, - toReference(args) { - return args.named; - } -}; +export default function(vm, args) { + return args.named; +} diff --git a/packages/ember-glimmer/lib/helpers/if-unless.js b/packages/ember-glimmer/lib/helpers/if-unless.js index ee3b91102bb..385243c4fd7 100644 --- a/packages/ember-glimmer/lib/helpers/if-unless.js +++ b/packages/ember-glimmer/lib/helpers/if-unless.js @@ -58,20 +58,17 @@ class ConditionalHelperReference extends CachedReference { @for Ember.Templates.helpers @public */ -export const inlineIf = { - isInternalHelper: true, - toReference({ positional: pargs }) { - switch (pargs.length) { - case 2: return ConditionalHelperReference.create(pargs.at(0), pargs.at(1), null); - case 3: return ConditionalHelperReference.create(pargs.at(0), pargs.at(1), pargs.at(2)); - default: - assert( - 'The inline form of the `if` helper expects two or three arguments, e.g. ' + - '`{{if trialExpired "Expired" expiryDate}}`.' - ); - } +export function inlineIf(vm, { positional }) { + switch (positional.length) { + case 2: return ConditionalHelperReference.create(positional.at(0), positional.at(1), null); + case 3: return ConditionalHelperReference.create(positional.at(0), positional.at(1), positional.at(2)); + default: + assert( + 'The inline form of the `if` helper expects two or three arguments, e.g. ' + + '`{{if trialExpired "Expired" expiryDate}}`.' + ); } -}; +} /** The inline `unless` helper conditionally renders a single property or string. @@ -89,17 +86,14 @@ export const inlineIf = { @for Ember.Templates.helpers @public */ -export const inlineUnless = { - isInternalHelper: true, - toReference({ positional: pargs }) { - switch (pargs.length) { - case 2: return ConditionalHelperReference.create(pargs.at(0), null, pargs.at(1)); - case 3: return ConditionalHelperReference.create(pargs.at(0), pargs.at(2), pargs.at(1)); - default: - assert( - 'The inline form of the `unless` helper expects two or three arguments, e.g. ' + - '`{{unless isFirstLogin "Welcome back!"}}`.' - ); - } +export function inlineUnless(vm, { positional }) { + switch (positional.length) { + case 2: return ConditionalHelperReference.create(positional.at(0), null, positional.at(1)); + case 3: return ConditionalHelperReference.create(positional.at(0), positional.at(2), positional.at(1)); + default: + assert( + 'The inline form of the `unless` helper expects two or three arguments, e.g. ' + + '`{{unless isFirstLogin "Welcome back!"}}`.' + ); } -}; +} diff --git a/packages/ember-glimmer/lib/helpers/loc.js b/packages/ember-glimmer/lib/helpers/loc.js index 2770fd04fb0..7548e13955c 100644 --- a/packages/ember-glimmer/lib/helpers/loc.js +++ b/packages/ember-glimmer/lib/helpers/loc.js @@ -1,4 +1,4 @@ -import { helper } from '../helper'; +import { InternalHelperReference } from '../utils/references'; import { String as StringUtils } from 'ember-runtime'; /** @@ -38,8 +38,10 @@ import { String as StringUtils } from 'ember-runtime'; @see {Ember.String#loc} @public */ -function locHelper(params) { - return StringUtils.loc.apply(null, params); +function locHelper({ positional }) { + return StringUtils.loc.apply(null, positional.value()); } -export default helper(locHelper); +export default function(vm, args) { + return new InternalHelperReference(locHelper, args); +} diff --git a/packages/ember-glimmer/lib/helpers/log.js b/packages/ember-glimmer/lib/helpers/log.js index 6273370f3e1..19466ba0395 100644 --- a/packages/ember-glimmer/lib/helpers/log.js +++ b/packages/ember-glimmer/lib/helpers/log.js @@ -1,4 +1,4 @@ -import { helper } from '../helper'; +import { InternalHelperReference } from '../utils/references'; /** @module ember @submodule ember-templates @@ -19,8 +19,10 @@ import Logger from 'ember-console'; @param {Array} params @public */ -function log(params) { - Logger.log.apply(null, params); +function log({ positional }) { + Logger.log.apply(null, positional.value()); } -export default helper(log); +export default function(vm, args) { + return new InternalHelperReference(log, args); +} diff --git a/packages/ember-glimmer/lib/helpers/mut.js b/packages/ember-glimmer/lib/helpers/mut.js index 876df838e8d..21d50a1596e 100644 --- a/packages/ember-glimmer/lib/helpers/mut.js +++ b/packages/ember-glimmer/lib/helpers/mut.js @@ -60,36 +60,32 @@ export function unMut(ref) { return ref[SOURCE] || ref; } -export default { - isInternalHelper: true, +export default function(vm, args) { + let rawRef = args.positional.at(0); - toReference(args) { - let rawRef = args.positional.at(0); - - if (isMut(rawRef)) { - return rawRef; - } - - // TODO: Improve this error message. This covers at least two distinct - // cases: - // - // 1. (mut "not a path") – passing a literal, result from a helper - // invocation, etc - // - // 2. (mut receivedValue) – passing a value received from the caller - // that was originally derived from a literal, result from a helper - // invocation, etc - // - // This message is alright for the first case, but could be quite - // confusing for the second case. - assert('You can only pass a path to mut', rawRef[UPDATE]); - - let wrappedRef = Object.create(rawRef); - - wrappedRef[SOURCE] = rawRef; - wrappedRef[INVOKE] = rawRef[UPDATE]; - wrappedRef[MUT_REFERENCE] = true; - - return wrappedRef; + if (isMut(rawRef)) { + return rawRef; } -}; + + // TODO: Improve this error message. This covers at least two distinct + // cases: + // + // 1. (mut "not a path") – passing a literal, result from a helper + // invocation, etc + // + // 2. (mut receivedValue) – passing a value received from the caller + // that was originally derived from a literal, result from a helper + // invocation, etc + // + // This message is alright for the first case, but could be quite + // confusing for the second case. + assert('You can only pass a path to mut', rawRef[UPDATE]); + + let wrappedRef = Object.create(rawRef); + + wrappedRef[SOURCE] = rawRef; + wrappedRef[INVOKE] = rawRef[UPDATE]; + wrappedRef[MUT_REFERENCE] = true; + + return wrappedRef; +} diff --git a/packages/ember-glimmer/lib/helpers/query-param.js b/packages/ember-glimmer/lib/helpers/query-param.js index 7dd767a7b4f..1057bda1741 100644 --- a/packages/ember-glimmer/lib/helpers/query-param.js +++ b/packages/ember-glimmer/lib/helpers/query-param.js @@ -10,9 +10,6 @@ function queryParams({ positional, named }) { }); } -export default { - isInternalHelper: true, - toReference(args) { - return new InternalHelperReference(queryParams, args); - } -}; +export default function(vm, args) { + return new InternalHelperReference(queryParams, args); +} diff --git a/packages/ember-glimmer/lib/helpers/readonly.js b/packages/ember-glimmer/lib/helpers/readonly.js index 76d18284793..b1224ad2b80 100644 --- a/packages/ember-glimmer/lib/helpers/readonly.js +++ b/packages/ember-glimmer/lib/helpers/readonly.js @@ -1,16 +1,12 @@ import { UPDATE } from '../utils/references'; import { unMut } from './mut'; -export default { - isInternalHelper: true, +export default function(vm, args) { + let ref = unMut(args.positional.at(0)); - toReference(args) { - let ref = unMut(args.positional.at(0)); + let wrapped = Object.create(ref); - let wrapped = Object.create(ref); + wrapped[UPDATE] = undefined; - wrapped[UPDATE] = undefined; - - return wrapped; - } -}; + return wrapped; +} diff --git a/packages/ember-glimmer/lib/helpers/unbound.js b/packages/ember-glimmer/lib/helpers/unbound.js index 471c2d02dfb..c6b2d970537 100644 --- a/packages/ember-glimmer/lib/helpers/unbound.js +++ b/packages/ember-glimmer/lib/helpers/unbound.js @@ -34,14 +34,11 @@ import { UnboundReference } from '../utils/references'; @public */ -export default { - isInternalHelper: true, - toReference(args) { - assert( - 'unbound helper cannot be called with multiple params or hash params', - args.positional.values.length === 1 && args.named.keys.length === 0 - ); - - return UnboundReference.create(args.positional.at(0).value()); - } -}; +export default function(vm, args) { + assert( + 'unbound helper cannot be called with multiple params or hash params', + args.positional.values.length === 1 && args.named.keys.length === 0 + ); + + return UnboundReference.create(args.positional.at(0).value()); +} From 1fdb766ab1844440fa7764cc43318131b7d018d1 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Wed, 7 Sep 2016 19:23:42 -0700 Subject: [PATCH 2/6] =?UTF-8?q?[GLIMMER]=20Fix=20sub-paths=20of=20{{#each-?= =?UTF-8?q?in}}=E2=80=99s=20values?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/ember-glimmer/lib/utils/iterable.js | 71 +++++++++++-------- .../tests/integration/syntax/each-in-test.js | 49 +++++++++++++ 2 files changed, 90 insertions(+), 30 deletions(-) diff --git a/packages/ember-glimmer/lib/utils/iterable.js b/packages/ember-glimmer/lib/utils/iterable.js index bf38e4d237f..97b83f7e456 100644 --- a/packages/ember-glimmer/lib/utils/iterable.js +++ b/packages/ember-glimmer/lib/utils/iterable.js @@ -156,7 +156,7 @@ class ObjectKeysIterator { this.position++; - return { key, value: memo, memo: value }; + return { key, value, memo }; } } @@ -172,36 +172,10 @@ class EmptyIterator { const EMPTY_ITERATOR = new EmptyIterator(); -class AbstractIterable { +class EachInIterable { constructor(ref, keyFor) { this.ref = ref; this.keyFor = keyFor; - } - - iterate() { - throw new Error('Not implemented: iterate'); - } - - valueReferenceFor(item) { - return new UpdatableReference(item.value); - } - - updateValueReference(reference, item) { - reference.update(item.value); - } - - memoReferenceFor(item) { - return new UpdatablePrimitiveReference(item.memo); - } - - updateMemoReference(reference, item) { - reference.update(item.memo); - } -} - -class EachInIterable extends AbstractIterable { - constructor(ref, keyFor) { - super(ref, keyFor); let valueTag = this.valueTag = new UpdatableTag(CONSTANT_TAG); @@ -229,13 +203,34 @@ class EachInIterable extends AbstractIterable { return EMPTY_ITERATOR; } } + + // {{each-in}} yields |key value| instead of |value key|, so the memo and + // value are flipped + + valueReferenceFor(item) { + return new UpdatablePrimitiveReference(item.memo); + } + + updateValueReference(reference, item) { + reference.update(item.memo); + } + + memoReferenceFor(item) { + return new UpdatableReference(item.value); + } + + updateMemoReference(reference, item) { + reference.update(item.value); + } } -class ArrayIterable extends AbstractIterable { +class ArrayIterable { constructor(ref, keyFor) { - super(ref, keyFor); + this.ref = ref; + this.keyFor = keyFor; let valueTag = this.valueTag = new UpdatableTag(CONSTANT_TAG); + this.tag = combine([ref.tag, valueTag]); } @@ -264,4 +259,20 @@ class ArrayIterable extends AbstractIterable { return EMPTY_ITERATOR; } } + + valueReferenceFor(item) { + return new UpdatableReference(item.value); + } + + updateValueReference(reference, item) { + reference.update(item.value); + } + + memoReferenceFor(item) { + return new UpdatablePrimitiveReference(item.memo); + } + + updateMemoReference(reference, item) { + reference.update(item.memo); + } } diff --git a/packages/ember-glimmer/tests/integration/syntax/each-in-test.js b/packages/ember-glimmer/tests/integration/syntax/each-in-test.js index 85645acc91e..37b8c8a238f 100644 --- a/packages/ember-glimmer/tests/integration/syntax/each-in-test.js +++ b/packages/ember-glimmer/tests/integration/syntax/each-in-test.js @@ -116,6 +116,55 @@ moduleFor('Syntax test: {{#each-in}}', class extends BasicEachInTest { `); } + [`@test it can render sub-paths of each item`]() { + this.render(strip` + + `, { + categories: { + 'Smartphones': { reports: { unitsSold: 8203 } }, + 'JavaScript Frameworks': { reports: { unitsSold: Infinity } } + } + }); + + this.assertHTML(strip` + + `); + + this.assertStableRerender(); + + this.runTask(() => { + set(this.context, 'categories.Smartphones.reports.unitsSold', 100); + set(this.context, 'categories.Tweets', { reports: { unitsSold: 443115 } }); + }); + + this.assertHTML(strip` + + `); + + this.runTask(() => set(this.context, 'categories', { + 'Smartphones': { reports: { unitsSold: 8203 } }, + 'JavaScript Frameworks': { reports: { unitsSold: Infinity } } + })); + + this.assertHTML(strip` + + `); + } + [`@test it can render duplicate items`]() { this.render(strip`