From 179a388d69af6fcd1bc906de396bde55e4925933 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Sun, 29 Aug 2021 07:26:49 -0500 Subject: [PATCH 1/3] [cleanup]: this. property fallback this-property-fallback --- .../interfaces/lib/compile/encoder.d.ts | 2 - .../lib/opcode-builder/helpers/resolution.ts | 11 ++--- .../opcode-compiler/lib/syntax/expressions.ts | 45 +------------------ .../opcode-compiler/lib/syntax/statements.ts | 19 -------- 4 files changed, 4 insertions(+), 73 deletions(-) diff --git a/packages/@glimmer/interfaces/lib/compile/encoder.d.ts b/packages/@glimmer/interfaces/lib/compile/encoder.d.ts index 3f245ad18..71c2fa50f 100644 --- a/packages/@glimmer/interfaces/lib/compile/encoder.d.ts +++ b/packages/@glimmer/interfaces/lib/compile/encoder.d.ts @@ -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; } ]; @@ -99,7 +98,6 @@ export type ResolveOptionalComponentOrHelperOp = [ ifComponent: (component: CompileTimeComponent) => void; ifHelper: (handle: number) => void; ifValue: (handle: number) => void; - ifFallback: (name: string) => void; } ]; diff --git a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts index 876496a44..e8154f2ce 100644 --- a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts +++ b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts @@ -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), @@ -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); } } @@ -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), @@ -396,10 +394,7 @@ export function resolveOptionalComponentOrHelper( if (helper !== null) { ifHelper(constants.helper(helper, name)); - return; } - - ifFallback(name); } } diff --git a/packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts b/packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts index ad455f713..836c23ccb 100644 --- a/packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts +++ b/packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts @@ -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(); @@ -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); }); @@ -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); - }, }); }); }); @@ -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); - }, }); }); }); diff --git a/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts b/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts index b61597629..2f3ce16d7 100644 --- a/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts +++ b/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts @@ -1,4 +1,3 @@ -import { deprecate } from '@glimmer/global-context'; import { CompileTimeComponent, ContentType, @@ -161,24 +160,6 @@ STATEMENTS.add(SexpOpcodes.Append, (op, [, value]) => { op(MachineOp.InvokeStatic, stdlibOperand('cautious-non-dynamic-append')); op(MachineOp.PopFrame); }, - - ifFallback(_name: string) { - op(MachineOp.PushFrame); - op(HighLevelResolutionOpcode.ResolveLocal, value[1], (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); - }); - op(MachineOp.InvokeStatic, stdlibOperand('cautious-append')); - op(MachineOp.PopFrame); - }, }); } else if (value[0] === SexpOpcodes.Call) { let [, expression, positional, named] = value; From f45ea5f27911e6e9c18684a752f479c61af3b72b Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Sun, 29 Aug 2021 08:31:57 -0500 Subject: [PATCH 2/3] Update some tests --- .../test/ember-component-test.ts | 35 +--------------- .../test/helpers/dynamic-helpers-test.ts | 23 ----------- .../integration-tests/test/updating-test.ts | 41 ++++++------------- 3 files changed, 15 insertions(+), 84 deletions(-) diff --git a/packages/@glimmer/integration-tests/test/ember-component-test.ts b/packages/@glimmer/integration-tests/test/ember-component-test.ts index 45e57eaeb..a8e431abd 100644 --- a/packages/@glimmer/integration-tests/test/ember-component-test.ts +++ b/packages/@glimmer/integration-tests/test/ember-component-test.ts @@ -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', `

{{@name}}

`); let subitems = [{ id: 0 }, { id: 1 }, { id: 42 }]; @@ -552,8 +552,7 @@ class CurlyScopeTest extends CurlyTest {
{{#each this.items key="id" as |item|}} - {{! Intentional property fallback to test self lookup }} - + {{/each}}
`, @@ -569,10 +568,6 @@ class CurlyScopeTest extends CurlyTest { ` ); - - assert.validateDeprecations( - /The `id` property was used in the `.*` template without using `this`/ - ); } @test @@ -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(''); - 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 { diff --git a/packages/@glimmer/integration-tests/test/helpers/dynamic-helpers-test.ts b/packages/@glimmer/integration-tests/test/helpers/dynamic-helpers-test.ts index 28c51a24b..33263e51a 100644 --- a/packages/@glimmer/integration-tests/test/helpers/dynamic-helpers-test.ts +++ b/packages/@glimmer/integration-tests/test/helpers/dynamic-helpers-test.ts @@ -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(''); - 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!'); diff --git a/packages/@glimmer/integration-tests/test/updating-test.ts b/packages/@glimmer/integration-tests/test/updating-test.ts index 395e9cc20..166210ed9 100644 --- a/packages/@glimmer/integration-tests/test/updating-test.ts +++ b/packages/@glimmer/integration-tests/test/updating-test.ts @@ -101,7 +101,7 @@ class UpdatingTest extends RenderTest { this.render( stripTight`
- [{{this.[]}}] + [{{this.['']}}] [{{this.[1]}}] [{{this.[undefined]}}] [{{this.[null]}}] @@ -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]}}] @@ -125,7 +125,7 @@ class UpdatingTest extends RenderTest { this.assertHTML(stripTight`
- [empty string] + [] [1] [undefined] [null] @@ -134,7 +134,7 @@ class UpdatingTest extends RenderTest { [this] [foo.bar] - [empty string] + [] [1] [undefined] [null] @@ -159,7 +159,7 @@ class UpdatingTest extends RenderTest { this.assertHTML(stripTight`
- [EMPTY STRING] + [] [ONE] [UNDEFINED] [NULL] @@ -168,7 +168,7 @@ class UpdatingTest extends RenderTest { [THIS] [FOO.BAR] - [EMPTY STRING] + [] [ONE] [UNDEFINED] [NULL] @@ -196,7 +196,7 @@ class UpdatingTest extends RenderTest { this.assertHTML(stripTight`
- [empty string] + [] [1] [undefined] [null] @@ -205,7 +205,7 @@ class UpdatingTest extends RenderTest { [this] [foo.bar] - [empty string] + [] [1] [undefined] [null] @@ -215,10 +215,6 @@ class UpdatingTest extends RenderTest { [foo.bar]
`); - - assert.validateDeprecations( - /The `` property was used in the `.*` template without using `this`/ - ); } @test @@ -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}}"; ----- @@ -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}}"; ----- @@ -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}} @@ -1101,19 +1097,12 @@ class UpdatingTest extends RenderTest {
`, '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('
{{#with this.person.name.first as |f|}}{{f}}{{/with}}{{f}}
', { + this.render('
{{#with this.person.name.first as |f|}}{{f}}{{/with}}{{this.f}}
', { person, f: 'Outer', }); @@ -1124,10 +1113,6 @@ class UpdatingTest extends RenderTest { this.rerender({ person }); this.assertHTML('
GodfreakOuter
', 'After updating'); - - assert.validateDeprecations( - /The `f` property was used in the `.*` template without using `this`/ - ); } @test From 89adeba133eba4fec3adad861c52ffba792de320 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Mon, 4 Oct 2021 16:48:42 -0500 Subject: [PATCH 3/3] fix perf benchark --- benchmark/benchmarks/krausest/lib/components/Row.hbs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmark/benchmarks/krausest/lib/components/Row.hbs b/benchmark/benchmarks/krausest/lib/components/Row.hbs index 9e2404933..21f1298de 100644 --- a/benchmark/benchmarks/krausest/lib/components/Row.hbs +++ b/benchmark/benchmarks/krausest/lib/components/Row.hbs @@ -1,6 +1,6 @@ {{@item.id}} - {{@item.label}} + {{@item.label}}