Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX beta] Prevent errors when using const (get arr 1). #16218

Merged
merged 1 commit into from
Feb 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions packages/ember-glimmer/lib/helpers/get.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,12 @@ class GetHelperReference extends CachedReference {

static create(sourceReference: VersionedPathReference<Opaque>, pathReference: PathReference<string>) {
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('.'));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I am working on a follow up PR (which will only target master) to make the referenceFromParts utility function accept a single value (instead of an array), that will remove this duplication (here and below)...

} else {
return sourceReference.get(value);
}
} else {
return new GetHelperReference(sourceReference, pathReference);
}
Expand Down Expand Up @@ -108,10 +112,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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ import Cache from './cache';
const firstDotIndexCache = new Cache(1000, key => key.indexOf('.'));

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
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ export function getPossibleMandatoryProxyValue(obj, keyName) {
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
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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