From ed4bef0992a06364dfc86013ff1dd3781127ad62 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Tue, 6 Feb 2018 12:41:59 -0500 Subject: [PATCH] [BUGFIX beta] Prevent errors when using const `(get arr 1)`. Prior to this change, calling `{{get arr 1}}` would throw an error: ``` pathReference.value(...).split is not a function ``` This commit fixes that, by only attempting to `.split` when the underlying value is not actually a string. `Ember.get` / `Ember.set` are also updated to allow `get(obj, 2)` (previously allowed only string keys). (cherry picked from commit 706166a3fa92707c40374a4614bb76aad0764810) --- packages/ember-glimmer/lib/helpers/get.ts | 14 ++++-- .../tests/integration/helpers/get-test.js | 50 +++++++++++++++++-- packages/ember-metal/lib/path_cache.js | 2 +- packages/ember-metal/lib/property_get.js | 4 +- packages/ember-metal/lib/property_set.js | 4 +- .../ember-metal/tests/accessors/get_test.js | 22 ++++++-- .../ember-metal/tests/accessors/set_test.js | 23 +++++++-- 7 files changed, 96 insertions(+), 23 deletions(-) diff --git a/packages/ember-glimmer/lib/helpers/get.ts b/packages/ember-glimmer/lib/helpers/get.ts index 5398f1c3042..1490594e6ed 100644 --- a/packages/ember-glimmer/lib/helpers/get.ts +++ b/packages/ember-glimmer/lib/helpers/get.ts @@ -77,8 +77,12 @@ class GetHelperReference extends CachedReference { static create(sourceReference: any, pathReference: PathReference) { if (isConst(pathReference)) { - let parts = pathReference.value().split('.'); - return referenceFromParts(sourceReference, parts); + let value = pathReference.value(); + if (typeof value === 'string' && value.indexOf('.') > -1) { + return referenceFromParts(sourceReference, value.split('.')); + } else { + return sourceReference.get(value); + } } else { return new GetHelperReference(sourceReference, pathReference); } @@ -106,10 +110,10 @@ class GetHelperReference extends CachedReference { if (path !== undefined && path !== null && path !== '') { let pathType = typeof path; - if (pathType === 'string') { + if (pathType === 'string' && path.indexOf('.') > -1) { innerReference = referenceFromParts(this.sourceReference, path.split('.')); - } else if (pathType === 'number') { - innerReference = this.sourceReference.get('' + path); + } else { + innerReference = this.sourceReference.get(path); } innerTag.inner.update(innerReference.tag); diff --git a/packages/ember-glimmer/tests/integration/helpers/get-test.js b/packages/ember-glimmer/tests/integration/helpers/get-test.js index cfd94b96324..723cbdeaba5 100644 --- a/packages/ember-glimmer/tests/integration/helpers/get-test.js +++ b/packages/ember-glimmer/tests/integration/helpers/get-test.js @@ -50,8 +50,8 @@ moduleFor('Helpers test: {{get}}', class extends RenderingTest { this.assertText('[red and yellow] [red and yellow]'); } - ['@test should be able to get an object value with numeric keys']() { - this.render(`{{#each indexes as |index|}}[{{get items index}}]{{/each}}`, { + ['@test should be able to get an object value with a number']() { + this.render(`[{{get items 1}}][{{get items 2}}][{{get items 3}}]`, { indexes: [1, 2, 3], items: { 1: 'First', @@ -75,8 +75,52 @@ moduleFor('Helpers test: {{get}}', class extends RenderingTest { this.assertText('[First][Second][Third]'); } + ['@test should be able to get an array value with a number']() { + this.render(`[{{get numbers 0}}][{{get numbers 1}}][{{get numbers 2}}]`, { + numbers: [1, 2, 3], + }); + + this.assertText('[1][2][3]'); + + this.runTask(() => this.rerender()); + + this.assertText('[1][2][3]'); + + this.runTask(() => set(this.context, 'numbers', [3, 2, 1])); + + this.assertText('[3][2][1]'); + + this.runTask(() => set(this.context, 'numbers', [1, 2, 3])); + + this.assertText('[1][2][3]'); + } + + ['@test should be able to get an object value with a path evaluating to a number']() { + this.render(`{{#each indexes as |index|}}[{{get items index}}]{{/each}}`, { + indexes: [1, 2, 3], + items: { + 1: 'First', + 2: 'Second', + 3: 'Third' + } + }); + + this.assertText('[First][Second][Third]'); + + this.runTask(() => this.rerender()); + + this.assertText('[First][Second][Third]'); + + this.runTask(() => set(this.context, 'items.1', 'Qux')); + + this.assertText('[Qux][Second][Third]'); + + this.runTask(() => set(this.context, 'items', { 1: 'First', 2: 'Second', 3: 'Third' })); + + this.assertText('[First][Second][Third]'); + } - ['@test should be able to get an array value with numeric keys']() { + ['@test should be able to get an array value with a path evaluating to a number']() { this.render(`{{#each numbers as |num index|}}[{{get numbers index}}]{{/each}}`, { numbers: [1, 2, 3], }); diff --git a/packages/ember-metal/lib/path_cache.js b/packages/ember-metal/lib/path_cache.js index 33da244ccee..57181d876a4 100644 --- a/packages/ember-metal/lib/path_cache.js +++ b/packages/ember-metal/lib/path_cache.js @@ -7,5 +7,5 @@ export const caches = { }; export function isPath(path) { - return firstDotIndexCache.get(path) !== -1; + return typeof path === 'string' && firstDotIndexCache.get(path) !== -1; } diff --git a/packages/ember-metal/lib/property_get.js b/packages/ember-metal/lib/property_get.js index b40bd7cb82b..b5e60baf019 100644 --- a/packages/ember-metal/lib/property_get.js +++ b/packages/ember-metal/lib/property_get.js @@ -54,8 +54,8 @@ const ALLOWABLE_TYPES = { export function get(obj, keyName) { assert(`Get must be called with two arguments; an object and a property key`, arguments.length === 2); assert(`Cannot call get with '${keyName}' on an undefined object.`, obj !== undefined && obj !== null); - assert(`The key provided to get must be a string, you passed ${keyName}`, typeof keyName === 'string'); - assert(`'this' in paths is not supported`, keyName.lastIndexOf('this.', 0) !== 0); + assert(`The key provided to get must be a string or number, you passed ${keyName}`, typeof keyName === 'string' || (typeof keyName === 'number' && !isNaN(keyName))); + assert(`'this' in paths is not supported`, typeof keyName !== 'string' || keyName.lastIndexOf('this.', 0) !== 0); assert('Cannot call `get` with an empty string', keyName !== ''); let type = typeof obj; diff --git a/packages/ember-metal/lib/property_set.js b/packages/ember-metal/lib/property_set.js index 47d4227f349..76e91ef303b 100644 --- a/packages/ember-metal/lib/property_set.js +++ b/packages/ember-metal/lib/property_set.js @@ -46,8 +46,8 @@ export function set(obj, keyName, value, tolerant) { arguments.length === 3 || arguments.length === 4 ); assert(`Cannot call set with '${keyName}' on an undefined object.`, obj && typeof obj === 'object' || typeof obj === 'function'); - assert(`The key provided to set must be a string, you passed ${keyName}`, typeof keyName === 'string'); - assert(`'this' in paths is not supported`, keyName.lastIndexOf('this.', 0) !== 0); + assert(`The key provided to set must be a string or number, you passed ${keyName}`, typeof keyName === 'string' || (typeof keyName === 'number' && !isNaN(keyName))); + assert(`'this' in paths is not supported`, typeof keyName !== 'string' || keyName.lastIndexOf('this.', 0) !== 0); assert(`calling set on destroyed object: ${toString(obj)}.${keyName} = ${toString(value)}`, !obj.isDestroyed); if (isPath(keyName)) { diff --git a/packages/ember-metal/tests/accessors/get_test.js b/packages/ember-metal/tests/accessors/get_test.js index aa084a9006c..aa226066362 100644 --- a/packages/ember-metal/tests/accessors/get_test.js +++ b/packages/ember-metal/tests/accessors/get_test.js @@ -27,6 +27,19 @@ moduleFor('get', class extends AbstractTestCase { } } + ['@test should retrieve a number key on an object'](assert) { + let obj = { 1: 'first' }; + + assert.equal(get(obj, 1), 'first'); + } + + ['@test should retrieve an array index'](assert) { + let arr = ['first', 'second']; + + assert.equal(get(arr, 0), 'first'); + assert.equal(get(arr, 1), 'second'); + } + ['@test should not access a property more than once'](assert) { let count = 0; let obj = { @@ -106,11 +119,10 @@ moduleFor('get', class extends AbstractTestCase { ['@test warn on attempts to use get with an unsupported property path']() { let obj = {}; - expectAssertion(() => get(obj, null), /The key provided to get must be a string, you passed null/); - expectAssertion(() => get(obj, NaN), /The key provided to get must be a string, you passed NaN/); - expectAssertion(() => get(obj, undefined), /The key provided to get must be a string, you passed undefined/); - expectAssertion(() => get(obj, false), /The key provided to get must be a string, you passed false/); - expectAssertion(() => get(obj, 42), /The key provided to get must be a string, you passed 42/); + expectAssertion(() => get(obj, null), /The key provided to get must be a string or number, you passed null/); + expectAssertion(() => get(obj, NaN), /The key provided to get must be a string or number, you passed NaN/); + expectAssertion(() => get(obj, undefined), /The key provided to get must be a string or number, you passed undefined/); + expectAssertion(() => get(obj, false), /The key provided to get must be a string or number, you passed false/); expectAssertion(() => get(obj, ''), /Cannot call `get` with an empty string/); } diff --git a/packages/ember-metal/tests/accessors/set_test.js b/packages/ember-metal/tests/accessors/set_test.js index b83949dfbdf..41c3262a5f6 100644 --- a/packages/ember-metal/tests/accessors/set_test.js +++ b/packages/ember-metal/tests/accessors/set_test.js @@ -34,6 +34,20 @@ moduleFor('set', class extends AbstractTestCase { } } + ['@test should set a number key on an object'](assert) { + let obj = { }; + + set(obj, 1, 'first'); + assert.equal(obj[1], 'first'); + } + + ['@test should set an array index'](assert) { + let arr = ['first', 'second']; + + set(arr, 1, 'lol'); + assert.deepEqual(arr, ['first', 'lol']); + } + ['@test should call setUnknownProperty if defined and value is undefined'](assert) { let obj = { count: 0, @@ -64,11 +78,10 @@ moduleFor('set', class extends AbstractTestCase { ['@test warn on attempts to use set with an unsupported property path']() { let obj = {}; - expectAssertion(() => set(obj, null, 42), /The key provided to set must be a string, you passed null/); - expectAssertion(() => set(obj, NaN, 42), /The key provided to set must be a string, you passed NaN/); - expectAssertion(() => set(obj, undefined, 42), /The key provided to set must be a string, you passed undefined/); - expectAssertion(() => set(obj, false, 42), /The key provided to set must be a string, you passed false/); - expectAssertion(() => set(obj, 42, 42), /The key provided to set must be a string, you passed 42/); + expectAssertion(() => set(obj, null, 42), /The key provided to set must be a string or number, you passed null/); + expectAssertion(() => set(obj, NaN, 42), /The key provided to set must be a string or number, you passed NaN/); + expectAssertion(() => set(obj, undefined, 42), /The key provided to set must be a string or number, you passed undefined/); + expectAssertion(() => set(obj, false, 42), /The key provided to set must be a string or number, you passed false/); } ['@test warn on attempts of calling set on a destroyed object']() {