Skip to content

Commit

Permalink
[BUGFIX beta] Prevent errors when using const (get arr 1).
Browse files Browse the repository at this point in the history
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 706166a)
  • Loading branch information
rwjblue committed Feb 14, 2018
1 parent 979e805 commit ed4bef0
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 23 deletions.
14 changes: 9 additions & 5 deletions packages/ember-glimmer/lib/helpers/get.ts
Expand Up @@ -77,8 +77,12 @@ class GetHelperReference extends CachedReference {

static create(sourceReference: any, pathReference: PathReference<any>) {
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);
}
Expand Down Expand Up @@ -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);
Expand Down
50 changes: 47 additions & 3 deletions packages/ember-glimmer/tests/integration/helpers/get-test.js
Expand Up @@ -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',
Expand All @@ -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],
});
Expand Down
2 changes: 1 addition & 1 deletion packages/ember-metal/lib/path_cache.js
Expand Up @@ -7,5 +7,5 @@ export const caches = {
};

export function isPath(path) {
return firstDotIndexCache.get(path) !== -1;
return typeof path === 'string' && firstDotIndexCache.get(path) !== -1;
}
4 changes: 2 additions & 2 deletions packages/ember-metal/lib/property_get.js
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions packages/ember-metal/lib/property_set.js
Expand Up @@ -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)) {
Expand Down
22 changes: 17 additions & 5 deletions packages/ember-metal/tests/accessors/get_test.js
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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/);
}

Expand Down
23 changes: 18 additions & 5 deletions packages/ember-metal/tests/accessors/set_test.js
Expand Up @@ -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,
Expand Down Expand Up @@ -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']() {
Expand Down

0 comments on commit ed4bef0

Please sign in to comment.