Skip to content

Commit

Permalink
Deprecate implicit this property lookup fallback
Browse files Browse the repository at this point in the history
Lands the deprecation proposed in
[emberjs/rfcs#308](https://github.com/emberjs/rfcs/blob/master/text/0308-deprecate-property-lookup-fallback.md):

The
[no-implicit-this](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-implicit-this.md)
template linting rule has been enabled for Ember applications by default
since around Ember 3.16 (included in the `octane` preset).

This PR will take that a step further and issue a deprecation for
templates that are still using `{{foo}}` to mean `{{this.foo}}`.

Note: It does provide for an easy way to temporarily disable the
deprecation messages (e.g. to reduce console output to hone in on other
issues). This is intended for sporadic usage while debugging **not** as
a way to mitigate the deprecation (the fallback behavior will be
removed in 4.0.0).
  • Loading branch information
rwjblue committed Feb 2, 2021
1 parent 8959f88 commit c793a41
Show file tree
Hide file tree
Showing 18 changed files with 269 additions and 164 deletions.
26 changes: 13 additions & 13 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
"@babel/plugin-transform-block-scoping": "^7.8.3",
"@babel/plugin-transform-object-assign": "^7.8.3",
"@ember/edition-utils": "^1.2.0",
"@glimmer/vm-babel-plugins": "0.74.0",
"@glimmer/vm-babel-plugins": "0.74.1",
"babel-plugin-debug-macros": "^0.3.3",
"babel-plugin-filter-imports": "^4.0.0",
"broccoli-concat": "^4.2.4",
Expand All @@ -75,19 +75,19 @@
},
"devDependencies": {
"@babel/preset-env": "^7.9.5",
"@glimmer/compiler": "0.74.0",
"@glimmer/compiler": "0.74.1",
"@glimmer/env": "^0.1.7",
"@glimmer/global-context": "0.74.0",
"@glimmer/interfaces": "0.74.0",
"@glimmer/manager": "0.74.0",
"@glimmer/destroyable": "0.74.0",
"@glimmer/owner": "0.74.0",
"@glimmer/node": "0.74.0",
"@glimmer/opcode-compiler": "0.74.0",
"@glimmer/program": "0.74.0",
"@glimmer/reference": "0.74.0",
"@glimmer/runtime": "0.74.0",
"@glimmer/validator": "0.74.0",
"@glimmer/global-context": "0.74.1",
"@glimmer/interfaces": "0.74.1",
"@glimmer/manager": "0.74.1",
"@glimmer/destroyable": "0.74.1",
"@glimmer/owner": "0.74.1",
"@glimmer/node": "0.74.1",
"@glimmer/opcode-compiler": "0.74.1",
"@glimmer/program": "0.74.1",
"@glimmer/reference": "0.74.1",
"@glimmer/runtime": "0.74.1",
"@glimmer/validator": "0.74.1",
"@simple-dom/document": "^1.4.0",
"@types/qunit": "^2.9.1",
"@types/rsvp": "^4.0.3",
Expand Down
17 changes: 17 additions & 0 deletions packages/@ember/-internals/environment/lib/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,23 @@ export const ENV = {
*/
_RERENDER_LOOP_LIMIT: 1000,

/**
Allows disabling the implicit this property fallback deprecation. This could be useful
as a way to control the volume of deprecations that are issued by temporarily disabling
the implicit this fallback deprecations, which would allow the other deprecations to be more easily
identified in the console).
NOTE: The fallback behavior **will be removed** in Ember 4.0.0, disabling **_IS NOT_**
a viable strategy for handling this deprecation.
@property _DISABLE_PROPERTY_FALLBACK_DEPRECATION
@for EmberENV
@type boolean
@default false
@private
*/
_DISABLE_PROPERTY_FALLBACK_DEPRECATION: false,

EMBER_LOAD_HOOKS: {} as {
[hook: string]: Function[];
},
Expand Down
19 changes: 17 additions & 2 deletions packages/@ember/-internals/glimmer/lib/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ setGlobalContext({

if (!override) throw new Error(`deprecation override for ${id} not found`);

deprecate(override.message ?? msg, Boolean(test), override);
// allow deprecations to be disabled in the VM_DEPRECATION_OVERRIDES array below
if (!override.disabled) {
deprecate(override.message ?? msg, Boolean(test), override);
}
}
},
});
Expand All @@ -91,7 +94,10 @@ if (DEBUG) {

// VM Assertion/Deprecation overrides

const VM_DEPRECATION_OVERRIDES: (DeprecationOptions & { message?: string })[] = [
const VM_DEPRECATION_OVERRIDES: (DeprecationOptions & {
disabled?: boolean;
message?: string;
})[] = [
{
id: 'autotracking.mutation-after-consumption',
until: '4.0.0',
Expand All @@ -100,6 +106,15 @@ const VM_DEPRECATION_OVERRIDES: (DeprecationOptions & { message?: string })[] =
enabled: '3.21.0',
},
},
{
id: 'this-property-fallback',
disabled: ENV._DISABLE_PROPERTY_FALLBACK_DEPRECATION,
until: '4.0.0',
for: 'ember-source',
since: {
enabled: '3.26.0',
},
},
];

const VM_ASSERTION_OVERRIDES: { id: string; message: string }[] = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ moduleFor(
}

['@test sharing a template between engine and application has separate refinements']() {
this.assert.expect(1);
this.assert.expect(2);

let sharedTemplate = compile(strip`
<h1>{{this.contextType}}</h1>
Expand All @@ -290,6 +290,10 @@ moduleFor(
{{outlet}}
`);

expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.add('template:application', sharedTemplate);
this.add(
'controller:application',
Expand Down Expand Up @@ -335,7 +339,11 @@ moduleFor(
}

['@test sharing a layout between engine and application has separate refinements']() {
this.assert.expect(1);
this.assert.expect(2);

expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

let sharedLayout = compile(strip`
{{ambiguous-curlies}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ moduleFor(
}

['@test it can access the model provided by the route via implicit this fallback']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.add(
'route:application',
Route.extend({
Expand Down Expand Up @@ -138,6 +142,10 @@ moduleFor(
}

async ['@test interior mutations on the model with set'](assert) {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.router.map(function () {
this.route('color', { path: '/:color' });
});
Expand Down Expand Up @@ -195,6 +203,10 @@ moduleFor(
}

async ['@test interior mutations on the model with tracked properties'](assert) {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

class Model {
@tracked color;

Expand Down Expand Up @@ -259,6 +271,10 @@ moduleFor(
}

async ['@test exterior mutations on the model with set'](assert) {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.router.map(function () {
this.route('color', { path: '/:color' });
});
Expand Down Expand Up @@ -316,6 +332,10 @@ moduleFor(
}

async ['@test exterior mutations on the model with tracked properties'](assert) {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.router.map(function () {
this.route('color', { path: '/:color' });
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ moduleFor(
this.registerComponent('foo-bar', { template: 'hello' });

this.render(
'{{foo-bar classNameBindings="model.someInitiallyTrueProperty model.someInitiallyFalseProperty model.someInitiallyUndefinedProperty :static model.isBig:big model.isOpen:open:closed model.isUp::down model.bar:isTruthy:isFalsy"}}',
'{{foo-bar classNameBindings="this.model.someInitiallyTrueProperty this.model.someInitiallyFalseProperty this.model.someInitiallyUndefinedProperty :static this.model.isBig:big this.model.isOpen:open:closed this.model.isUp::down this.model.bar:isTruthy:isFalsy"}}',
{
model: {
someInitiallyTrueProperty: true,
Expand Down Expand Up @@ -344,7 +344,7 @@ moduleFor(

['@test const bindings can be set as attrs']() {
this.registerComponent('foo-bar', { template: 'hello' });
this.render('{{foo-bar classNameBindings="foo:enabled:disabled"}}', {
this.render('{{foo-bar classNameBindings="this.foo:enabled:disabled"}}', {
foo: true,
});

Expand Down Expand Up @@ -697,7 +697,7 @@ moduleFor(
['@test it should merge classBinding with class']() {
this.registerComponent('foo-bar', { template: 'hello' });

this.render('{{foo-bar classBinding="birdman:respeck" class="myName"}}', {
this.render('{{foo-bar classBinding="this.birdman:respeck" class="myName"}}', {
birdman: true,
});

Expand All @@ -719,7 +719,7 @@ moduleFor(
['@test it should apply classBinding with only truthy condition']() {
this.registerComponent('foo-bar', { template: 'hello' });

this.render('{{foo-bar classBinding="myName:respeck"}}', {
this.render('{{foo-bar classBinding="this.myName:respeck"}}', {
myName: true,
});

Expand All @@ -741,7 +741,7 @@ moduleFor(
['@test it should apply classBinding with only falsy condition']() {
this.registerComponent('foo-bar', { template: 'hello' });

this.render('{{foo-bar classBinding="myName::shade"}}', {
this.render('{{foo-bar classBinding="this.myName::shade"}}', {
myName: false,
});

Expand All @@ -763,7 +763,7 @@ moduleFor(
['@test it should apply nothing when classBinding is falsy but only supplies truthy class']() {
this.registerComponent('foo-bar', { template: 'hello' });

this.render('{{foo-bar classBinding="myName:respeck"}}', {
this.render('{{foo-bar classBinding="this.myName:respeck"}}', {
myName: false,
});

Expand All @@ -785,7 +785,7 @@ moduleFor(
['@test it should apply nothing when classBinding is truthy but only supplies falsy class']() {
this.registerComponent('foo-bar', { template: 'hello' });

this.render('{{foo-bar classBinding="myName::shade"}}', { myName: true });
this.render('{{foo-bar classBinding="this.myName::shade"}}', { myName: true });

this.assertComponentElement(this.firstChild, {
tagName: 'div',
Expand All @@ -805,7 +805,7 @@ moduleFor(
['@test it should apply classBinding with falsy condition']() {
this.registerComponent('foo-bar', { template: 'hello' });

this.render('{{foo-bar classBinding="swag:fresh:scrub"}}', {
this.render('{{foo-bar classBinding="this.swag:fresh:scrub"}}', {
swag: false,
});

Expand All @@ -827,7 +827,7 @@ moduleFor(
['@test it should apply classBinding with truthy condition']() {
this.registerComponent('foo-bar', { template: 'hello' });

this.render('{{foo-bar classBinding="swag:fresh:scrub"}}', {
this.render('{{foo-bar classBinding="this.swag:fresh:scrub"}}', {
swag: true,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1501,7 +1501,7 @@ applyMixins(
title: 'hash value',
setup() {
this.registerComponent('my-comp', {
template: '{{component component}}',
template: '{{component this.component}}',
});

this.render('{{my-comp component=(component "change-button" val=this.model.val2)}}');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3734,6 +3734,10 @@ moduleFor(
}

['@test can use `{{component.foo}}` in a template GH#19313']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.registerComponent('foo-bar', {
template: '{{component.foo}}',
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -985,7 +985,7 @@ moduleFor(
'Did you mean `<Input @type="checkbox" @checked={{...}} />`?';

expectWarning(() => {
this.render(`<Input @type="checkbox" @value={{value}} />`, {
this.render(`<Input @type="checkbox" @value={{this.value}} />`, {
value: true,
});
}, message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,7 @@ moduleFor(
'Did you mean `<Input @type="checkbox" @checked={{...}} />`?';

expectWarning(() => {
this.render(`{{input type="checkbox" value=value}}`, {
this.render(`{{input type="checkbox" value=this.value}}`, {
value: true,
});
}, message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ moduleFor(
'index',
`
<h3 class="home">Home</h3>
{{#link-to 'about' id='about-link' classNameBindings='foo:foo-is-true:foo-is-false'}}About{{/link-to}}
{{#link-to 'about' id='about-link' classNameBindings='this.foo:foo-is-true:foo-is-false'}}About{{/link-to}}
`
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ if (ENV._TEMPLATE_ONLY_GLIMMER_COMPONENTS) {
['@test it can render named arguments']() {
this.registerTemplateOnlyComponent('foo-bar', '|{{@foo}}|{{@bar}}|');

this.render('{{foo-bar foo=foo bar=bar}}', {
this.render('{{foo-bar foo=this.foo bar=this.bar}}', {
foo: 'foo',
bar: 'bar',
});
Expand All @@ -197,9 +197,13 @@ if (ENV._TEMPLATE_ONLY_GLIMMER_COMPONENTS) {
}

['@test it renders named arguments as reflected properties']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.registerTemplateOnlyComponent('foo-bar', '|{{foo}}|{{this.bar}}|');

this.render('{{foo-bar foo=foo bar=bar}}', {
this.render('{{foo-bar foo=this.foo bar=this.bar}}', {
foo: 'foo',
bar: 'bar',
});
Expand All @@ -224,7 +228,7 @@ if (ENV._TEMPLATE_ONLY_GLIMMER_COMPONENTS) {
['@test it has curly component features']() {
this.registerTemplateOnlyComponent('foo-bar', 'hello');

this.render('{{foo-bar tagName="p" class=class}}', {
this.render('{{foo-bar tagName="p" class=this.class}}', {
class: 'foo bar',
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ moduleFor(
}

['@test it does not resolve helpers with a `.` (period)']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.registerHelper('hello.world', () => 'hello world');

this.render('{{hello.world}}', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,10 @@ class EachTest extends AbstractEachTest {
}

['@test the scoped variable is not available outside the {{#each}} block.']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.makeList(['Yehuda']);

this.render(`{{name}}-{{#each this.list as |name|}}{{name}}{{/each}}-{{name}}`, {
Expand Down Expand Up @@ -948,6 +952,10 @@ class EachTest extends AbstractEachTest {
}

['@test the scoped variable is not available outside the {{#each}} block']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

let first = this.createList(['Limbo']);
let fifth = this.createList(['Wrath']);
let ninth = this.createList(['Treachery']);
Expand Down
Loading

0 comments on commit c793a41

Please sign in to comment.