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

[cleanup]: Remove this. property fallback (e.g. the this-property-fallback deprecation) #1331

Merged
merged 3 commits into from Oct 5, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmark/benchmarks/krausest/lib/components/Row.hbs
@@ -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
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
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
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]
[]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is different behavior. Not sure if technically a regression though. Neither this.[] nor this[''] work.

[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
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
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
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);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this whole thing be ✂️?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't tell if GetFreeAsFallback also handles the case of an in scope "loose/free" variable. @rwjblue

Copy link
Member

Choose a reason for hiding this comment

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

@snewcomer - Let's land this as is now, and maybe you can send a follow up PR that removes the GetFreeAsFallback op code and see if all tests pass...


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