Skip to content

Commit

Permalink
Merge pull request #1331 from snewcomer/sn/this-fallback
Browse files Browse the repository at this point in the history
  • Loading branch information
@rwjblue
rwjblue authored Oct 5, 2021
2 parents 847dec7 + 89adeba commit ed22236
Show file tree
Hide file tree
Showing 8 changed files with 20 additions and 158 deletions.
2 changes: 1 addition & 1 deletion benchmark/benchmarks/krausest/lib/components/Row.hbs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<tr class={{if @item.selected "danger"}}>
<td class="col-md-1">{{@item.id}}</td>
<td class="col-md-4"><a {{on "click" onSelect}}>{{@item.label}}</a></td>
<td class="col-md-4"><a {{on "click" this.onSelect}}>{{@item.label}}</a></td>
<td class="col-md-1"><a><span class="glyphicon glyphicon-remove" aria-hidden="true"></span></a></td>
<td class="col-md-6"></td>
</tr>
35 changes: 2 additions & 33 deletions packages/@glimmer/integration-tests/test/ember-component-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ class CurlyScopeTest extends CurlyTest {
}

@test
'correct scope - self lookup inside #each'(assert: Assert) {
'correct scope - self lookup inside #each'() {
this.registerComponent('TemplateOnly', 'SubItem', `<p>{{@name}}</p>`);

let subitems = [{ id: 0 }, { id: 1 }, { id: 42 }];
Expand All @@ -552,8 +552,7 @@ class CurlyScopeTest extends CurlyTest {
<div>
{{#each this.items key="id" as |item|}}
<SubItem @name={{this.id}} />
{{! Intentional property fallback to test self lookup }}
<SubItem @name={{id}} />
<SubItem @name={{this.id}} />
<SubItem @name={{item.id}} />
{{/each}}
</div>`,
Expand All @@ -569,10 +568,6 @@ class CurlyScopeTest extends CurlyTest {
</div>
`
);

assert.validateDeprecations(
/The `id` property was used in the `.*` template without using `this`/
);
}

@test
Expand Down Expand Up @@ -1633,32 +1628,6 @@ class CurlyGlimmerComponentTest extends CurlyTest {
this.assertHTML('Foo');
this.assertStableNodes();
}

@test
'Can use implicit this fallback for `component.name` emberjs/ember.js#19313'(assert: Assert) {
this.registerComponent(
'Glimmer',
'Outer',
'{{component.name}}',
class extends GlimmerishComponent {
get component() {
return { name: 'Foo' };
}
}
);

this.render('<Outer />');
this.assertHTML('Foo');

this.rerender();

this.assertHTML('Foo');
this.assertStableNodes();

assert.validateDeprecations(
/The `component\.name` property path was used in the `.*` template without using `this`/
);
}
}

class CurlyTeardownTest extends CurlyTest {
Expand Down
23 changes: 0 additions & 23 deletions packages/@glimmer/integration-tests/test/helpers/dynamic-helpers-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,6 @@ class DynamicHelpersResolutionModeTest extends RenderTest {
this.assertStableRerender();
}

@test
'Can invoke a helper definition based on this fallback lookup in resolution mode'(
assert: Assert
) {
const foo = defineSimpleHelper(() => 'Hello, world!');
this.registerComponent(
'Glimmer',
'Bar',
'{{x.foo}}',
class extends GlimmerishComponent {
x = { foo };
}
);

this.render('<Bar/>');
this.assertHTML('Hello, world!');
this.assertStableRerender();

assert.validateDeprecations(
/The `x\.foo` property path was used in the `.*` template without using `this`/
);
}

@test
'Can use a dynamic helper with nested helpers'() {
const foo = defineSimpleHelper(() => 'world!');
Expand Down
41 changes: 13 additions & 28 deletions packages/@glimmer/integration-tests/test/updating-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class UpdatingTest extends RenderTest {
this.render(
stripTight`
<div>
[{{this.[]}}]
[{{this.['']}}]
[{{this.[1]}}]
[{{this.[undefined]}}]
[{{this.[null]}}]
Expand All @@ -110,7 +110,7 @@ class UpdatingTest extends RenderTest {
[{{this.[this]}}]
[{{this.[foo.bar]}}]
[{{this.nested.[]}}]
[{{this.nested.['']}}]
[{{this.nested.[1]}}]
[{{this.nested.[undefined]}}]
[{{this.nested.[null]}}]
Expand All @@ -125,7 +125,7 @@ class UpdatingTest extends RenderTest {

this.assertHTML(stripTight`
<div>
[empty string]
[]
[1]
[undefined]
[null]
Expand All @@ -134,7 +134,7 @@ class UpdatingTest extends RenderTest {
[this]
[foo.bar]
[empty string]
[]
[1]
[undefined]
[null]
Expand All @@ -159,7 +159,7 @@ class UpdatingTest extends RenderTest {

this.assertHTML(stripTight`
<div>
[EMPTY STRING]
[]
[ONE]
[UNDEFINED]
[NULL]
Expand All @@ -168,7 +168,7 @@ class UpdatingTest extends RenderTest {
[THIS]
[FOO.BAR]
[EMPTY STRING]
[]
[ONE]
[UNDEFINED]
[NULL]
Expand Down Expand Up @@ -196,7 +196,7 @@ class UpdatingTest extends RenderTest {

this.assertHTML(stripTight`
<div>
[empty string]
[]
[1]
[undefined]
[null]
Expand All @@ -205,7 +205,7 @@ class UpdatingTest extends RenderTest {
[this]
[foo.bar]
[empty string]
[]
[1]
[undefined]
[null]
Expand All @@ -215,10 +215,6 @@ class UpdatingTest extends RenderTest {
[foo.bar]
</div>
`);

assert.validateDeprecations(
/The `` property was used in the `.*` template without using `this`/
);
}

@test
Expand Down Expand Up @@ -935,8 +931,8 @@ class UpdatingTest extends RenderTest {
foo: "{{foo}}";
bar: "{{bar}}";
value: "{{this.value}}";
echo foo: "{{echo foo}}";
echo bar: "{{echo bar}}";
echo foo: "{{echo this.foo}}";
echo bar: "{{echo this.bar}}";
echo value: "{{echo this.value}}";
-----
Expand All @@ -946,7 +942,7 @@ class UpdatingTest extends RenderTest {
bar: "{{bar}}";
value: "{{this.value}}";
echo foo: "{{echo foo}}";
echo bar: "{{echo bar}}";
echo bar: "{{echo this.bar}}";
echo value: "{{echo this.value}}";
-----
Expand All @@ -967,7 +963,7 @@ class UpdatingTest extends RenderTest {
foo: "{{foo}}";
bar: "{{bar}}";
value: "{{this.value}}";
echo foo: "{{echo foo}}";
echo foo: "{{echo this.foo}}";
echo bar: "{{echo bar}}";
echo value: "{{echo this.value}}";
{{/with}}
Expand Down Expand Up @@ -1101,19 +1097,12 @@ class UpdatingTest extends RenderTest {
</div>`,
'After reset'
);

assert.validateDeprecations(
/The `foo` property path was used in the `.*` template without using `this`/,
/The `bar` property path was used in the `.*` template without using `this`/,
/The `bar` property path was used in the `.*` template without using `this`/,
/The `foo` property path was used in the `.*` template without using `this`/
);
}

@test
'block arguments (ensure balanced push/pop)'() {
let person = { name: { first: 'Godfrey', last: 'Chan' } };
this.render('<div>{{#with this.person.name.first as |f|}}{{f}}{{/with}}{{f}}</div>', {
this.render('<div>{{#with this.person.name.first as |f|}}{{f}}{{/with}}{{this.f}}</div>', {
person,
f: 'Outer',
});
Expand All @@ -1124,10 +1113,6 @@ class UpdatingTest extends RenderTest {
this.rerender({ person });

this.assertHTML('<div>GodfreakOuter</div>', 'After updating');

assert.validateDeprecations(
/The `f` property was used in the `.*` template without using `this`/
);
}

@test
Expand Down
2 changes: 0 additions & 2 deletions packages/@glimmer/interfaces/lib/compile/encoder.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ export type ResolveOptionalHelperOp = [
op1: WireFormat.Expressions.Expression,
op2: {
ifHelper: (handle: number, name: string, moduleName: string) => void;
ifFallback: (name: string, moduleName: string) => void;
}
];

Expand All @@ -99,7 +98,6 @@ export type ResolveOptionalComponentOrHelperOp = [
ifComponent: (component: CompileTimeComponent) => void;
ifHelper: (handle: number) => void;
ifValue: (handle: number) => void;
ifFallback: (name: string) => void;
}
];

Expand Down
11 changes: 3 additions & 8 deletions packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ export function resolveOptionalHelper(
resolver: CompileTimeResolver,
constants: CompileTimeConstants & ResolutionTimeConstants,
meta: ContainingMetadata,
[, expr, { ifHelper, ifFallback }]: ResolveOptionalHelperOp
[, expr, { ifHelper }]: ResolveOptionalHelperOp
): void {
assert(
isGetFreeOptionalHelper(expr) || isGetFreeDeprecatedHelper(expr),
Expand All @@ -320,9 +320,7 @@ export function resolveOptionalHelper(
let name = upvars[expr[1]];
let helper = resolver.lookupHelper(name, owner);

if (helper === null) {
ifFallback(name, meta.moduleName);
} else {
if (helper) {
ifHelper(constants.helper(helper, name), name, meta.moduleName);
}
}
Expand All @@ -334,7 +332,7 @@ export function resolveOptionalComponentOrHelper(
resolver: CompileTimeResolver,
constants: CompileTimeConstants & ResolutionTimeConstants,
meta: ContainingMetadata,
[, expr, { ifComponent, ifHelper, ifValue, ifFallback }]: ResolveOptionalComponentOrHelperOp
[, expr, { ifComponent, ifHelper, ifValue }]: ResolveOptionalComponentOrHelperOp
): void {
assert(
isGetFreeOptionalComponentOrHelper(expr),
Expand Down Expand Up @@ -396,10 +394,7 @@ export function resolveOptionalComponentOrHelper(

if (helper !== null) {
ifHelper(constants.helper(helper, name));
return;
}

ifFallback(name);
}
}

Expand Down
45 changes: 1 addition & 44 deletions packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { isGetFreeHelper } from '../opcode-builder/helpers/resolution';
import { SimpleArgs } from '../opcode-builder/helpers/shared';
import { Call, CallDynamic, Curry, PushPrimitiveReference } from '../opcode-builder/helpers/vm';
import { Compilers, PushExpressionOp } from './compilers';
import { DEBUG } from '@glimmer/env';

export const EXPRESSIONS = new Compilers<PushExpressionOp, ExpressionSexpOpcode>();

Expand Down Expand Up @@ -57,23 +56,7 @@ EXPRESSIONS.add(SexpOpcodes.GetStrictFree, (op, [, sym, _path]) => {
});
});

EXPRESSIONS.add(SexpOpcodes.GetFreeAsFallback, (op, [, freeVar, path]) => {
op(HighLevelResolutionOpcode.ResolveLocal, freeVar, (name: string, moduleName: string) => {
if (DEBUG) {
let propertyPath = path ? [name, ...path].join('.') : name;

deprecate(
`The \`${propertyPath}\` property path was used in the \`${moduleName}\` template without using \`this\`. This fallback behavior has been deprecated, all properties must be looked up on \`this\` when used in the template: {{this.${propertyPath}}}`,
false,
{
id: 'this-property-fallback',
}
);
}

op(Op.GetVariable, 0);
op(Op.GetProperty, name);
});
EXPRESSIONS.add(SexpOpcodes.GetFreeAsFallback, (op, [, , path]) => {
withPath(op, path);
});

Expand All @@ -93,19 +76,6 @@ EXPRESSIONS.add(SexpOpcodes.GetFreeAsHelperHeadOrThisFallback, (op, expr) => {
ifHelper: (handle: number) => {
Call(op, handle, null, null);
},

ifFallback: (name: string, moduleName: string) => {
deprecate(
`The \`${name}\` property was used in the \`${moduleName}\` template without using \`this\`. This fallback behavior has been deprecated, all properties must be looked up on \`this\` when used in the template: {{this.${name}}}`,
false,
{
id: 'this-property-fallback',
}
);

op(Op.GetVariable, 0);
op(Op.GetProperty, name);
},
});
});
});
Expand Down Expand Up @@ -140,19 +110,6 @@ EXPRESSIONS.add(SexpOpcodes.GetFreeAsDeprecatedHelperHeadOrThisFallback, (op, ex

Call(op, handle, null, null);
},

ifFallback: (name: string, moduleName: string) => {
deprecate(
`The \`${name}\` property was used in the \`${moduleName}\` template without using \`this\`. This fallback behavior has been deprecated, all properties must be looked up on \`this\` when used in the template: {{this.${name}}}`,
false,
{
id: 'this-property-fallback',
}
);

op(Op.GetVariable, 0);
op(Op.GetProperty, name);
},
});
});
});
Expand Down
Loading

0 comments on commit ed22236

Please sign in to comment.