From a8d96bffbc35d14fb1bca2c4174863fe47619a5c Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Tue, 13 Feb 2024 15:07:18 -0500 Subject: [PATCH 1/8] Add test for conditional on to try to reproduce https://github.com/emberjs/ember.js/issues/20647 --- .../test/modifiers/on-test.ts | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts index ad43550b90..d81cf1835f 100644 --- a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts @@ -315,11 +315,37 @@ if (hasDom) { this.assertCounts({ adds: 1, removes: 0 }); } + @test + 'updates to a (remaining truthy) condition do not leave the element without an attached modifier'(assert: + Assert) { + let calledCount = 0; + + this.render('', { + callback() { + calledCount++; + }, + condition: true, + }); + + this.findButton().click(); + assert.strictEqual(calledCount, 1, 'callback is being invoked'); + + this.rerender({ condition: true }); + + this.findButton().click(); + assert.strictEqual(calledCount, 2, 'callback is being invoked'); + + this.rerender({ condition: true }); + + this.findButton().click(); + assert.strictEqual(calledCount, 3, 'callback is being invoked'); + } + @test 'asserts when eventName is missing'(assert: Assert) { assert.throws(() => { this.render(``, { - callback() {}, + callback() { }, }); }, /You must pass a valid DOM event name as the first argument to the `on` modifier/u); } @@ -328,7 +354,7 @@ if (hasDom) { 'asserts when eventName is a bound undefined value'(assert: Assert) { assert.throws(() => { this.render(``, { - callback() {}, + callback() { }, }); }, /You must pass a valid DOM event name as the first argument to the `on` modifier/u); } @@ -337,7 +363,7 @@ if (hasDom) { 'asserts when eventName is a function'(assert: Assert) { assert.throws(() => { this.render(``, { - callback() {}, + callback() { }, }); }, /You must pass a valid DOM event name as the first argument to the `on` modifier/u); } @@ -384,7 +410,7 @@ if (hasDom) { 'asserts if more than 2 positional parameters are provided'(assert: Assert) { assert.throws(() => { this.render(``, { - callback() {}, + callback() { }, someArg: 'foo', }); }, /You can only pass two positional arguments \(event name and callback\) to the `on` modifier, but you provided 3. Consider using the `fn` helper to provide additional arguments to the `on` callback./u); From 47276940915ff418e8b91c31358d5950f03906a3 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Tue, 13 Feb 2024 15:19:54 -0500 Subject: [PATCH 2/8] oh no why --- .../integration-tests/test/modifiers/on-test.ts | 8 +++++++- packages/@glimmer/runtime/lib/modifiers/on.ts | 10 ++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts index d81cf1835f..56b205307b 100644 --- a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts @@ -320,10 +320,16 @@ if (hasDom) { Assert) { let calledCount = 0; - this.render('', { + this.render(` + `, { callback() { calledCount++; }, + on, condition: true, }); diff --git a/packages/@glimmer/runtime/lib/modifiers/on.ts b/packages/@glimmer/runtime/lib/modifiers/on.ts index fd6f0b3427..256dc6b5fb 100644 --- a/packages/@glimmer/runtime/lib/modifiers/on.ts +++ b/packages/@glimmer/runtime/lib/modifiers/on.ts @@ -86,9 +86,8 @@ export class OnModifierState { valueForRef(userProvidedCallbackReference), CheckFunction, (actual) => { - return `You must pass a function as the second argument to the \`on\` modifier; you passed ${ - actual === null ? 'null' : typeof actual - }. While rendering:\n\n${userProvidedCallbackReference.debugLabel ?? `{unlabeled value}`}`; + return `You must pass a function as the second argument to the \`on\` modifier; you passed ${actual === null ? 'null' : typeof actual + }. While rendering:\n\n${userProvidedCallbackReference.debugLabel ?? `{unlabeled value}`}`; } ) as EventListener; @@ -107,12 +106,11 @@ export class OnModifierState { if (this.shouldUpdate) { if (needsCustomCallback) { - this.callback = function (this: Element, event) { + this.callback = function(this: Element, event) { if (import.meta.env.DEV && passive) { event.preventDefault = () => { throw new Error( - `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${ - userProvidedCallback.name ?? `{anonymous function}` + `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${userProvidedCallback.name ?? `{anonymous function}` }` ); }; From 79e7055a0256e6a762980f8f208c864cd08af720 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 16 Feb 2024 17:01:05 -0500 Subject: [PATCH 3/8] Fix issue with dynamic modifiers infinitely receiving duplicate arguments --- .../test/modifiers/dynamic-modifiers-test.ts | 22 +++++++++++++ .../test/modifiers/on-test.ts | 32 ------------------- .../runtime/lib/compiled/opcodes/dom.ts | 14 ++++---- 3 files changed, 29 insertions(+), 39 deletions(-) diff --git a/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts b/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts index 4b5e4250f9..31f398ca5f 100644 --- a/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts @@ -65,6 +65,28 @@ class DynamicModifiersResolutionModeTest extends RenderTest { this.assertStableRerender(); } + @test + 'Modifiers with dynamic arguments receive the correct number of arguments'(assert) { + let receivedArgs: unknown[] = []; + const foo = defineSimpleModifier( + (_element: unknown, args: unknown[]) => (receivedArgs = args) + ); + + this.render(` + {{~#let (modifier this.foo this.outer) as |foo|~}} +
General Kenobi!
+ {{~/let~}} + `, { foo, inner: 'x', outer: 'y', cond: true }); + + this.assertHTML('
General Kenobi!
'); + this.assertStableRerender(); + assert.deepEqual(receivedArgs, ['y', 'x']); + this.rerender({ cond: false }); + this.rerender({ cond: true }); + assert.deepEqual(receivedArgs, ['y', 'x']); + + } + @test 'Can pass curried modifier as argument and invoke dynamically (with args)'() { const foo = defineSimpleModifier( diff --git a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts index 56b205307b..bda4f7984f 100644 --- a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts @@ -315,38 +315,6 @@ if (hasDom) { this.assertCounts({ adds: 1, removes: 0 }); } - @test - 'updates to a (remaining truthy) condition do not leave the element without an attached modifier'(assert: - Assert) { - let calledCount = 0; - - this.render(` - `, { - callback() { - calledCount++; - }, - on, - condition: true, - }); - - this.findButton().click(); - assert.strictEqual(calledCount, 1, 'callback is being invoked'); - - this.rerender({ condition: true }); - - this.findButton().click(); - assert.strictEqual(calledCount, 2, 'callback is being invoked'); - - this.rerender({ condition: true }); - - this.findButton().click(); - assert.strictEqual(calledCount, 3, 'callback is being invoked'); - } - @test 'asserts when eventName is missing'(assert: Assert) { assert.throws(() => { diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts index 5d7d757972..79633d5e8b 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts @@ -22,7 +22,7 @@ import { import { associateDestroyableChild, destroy } from '@glimmer/destroyable'; import { getInternalModifierManager } from '@glimmer/manager'; import { createComputeRef, isConstRef, valueForRef } from '@glimmer/reference'; -import { assign, debugToString, expect, isObject } from '@glimmer/util'; +import { debugToString, expect, isObject } from '@glimmer/util'; import { consumeTag, CURRENT_TAG, validateTag, valueForTag } from '@glimmer/validator'; import { $t0, CurriedTypes, Op } from '@glimmer/vm'; @@ -153,6 +153,8 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => { let { stack } = vm; let ref = check(stack.pop(), CheckReference); let args = check(stack.pop(), CheckArguments).capture(); + let { positional: outerPositional, named: outerNamed } = args; + let { constructing } = vm.elements(); let initialOwner = vm.getOwner(); @@ -178,11 +180,11 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => { owner = curriedOwner; if (positional !== undefined) { - args.positional = positional.concat(args.positional) as CapturedPositionalArguments; + args.positional = [...positional, ...outerPositional] as CapturedPositionalArguments; } if (named !== undefined) { - args.named = assign({}, ...named, args.named); + args.named = Object.assign({}, named, outerNamed); } } else { hostDefinition = value; @@ -194,10 +196,8 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => { if (manager === null) { if (import.meta.env.DEV) { throw new Error( - `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${ - ref.debugLabel - }}}\`, and the incorrect definition is the value at the path \`${ - ref.debugLabel + `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${ref.debugLabel + }}}\`, and the incorrect definition is the value at the path \`${ref.debugLabel }\`, which was: ${debugToString!(hostDefinition)}` ); } else { From 891b31d407c5cef3c7dee0025c93932d8c47702d Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 16 Feb 2024 17:14:55 -0500 Subject: [PATCH 4/8] lint:fix --- .../test/modifiers/dynamic-modifiers-test.ts | 12 ++++++------ .../integration-tests/test/modifiers/on-test.ts | 8 ++++---- .../@glimmer/runtime/lib/compiled/opcodes/dom.ts | 6 ++++-- packages/@glimmer/runtime/lib/modifiers/on.ts | 10 ++++++---- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts b/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts index 31f398ca5f..66c45c5448 100644 --- a/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts @@ -68,15 +68,16 @@ class DynamicModifiersResolutionModeTest extends RenderTest { @test 'Modifiers with dynamic arguments receive the correct number of arguments'(assert) { let receivedArgs: unknown[] = []; - const foo = defineSimpleModifier( - (_element: unknown, args: unknown[]) => (receivedArgs = args) - ); + const foo = defineSimpleModifier((_element: unknown, args: unknown[]) => (receivedArgs = args)); - this.render(` + this.render( + ` {{~#let (modifier this.foo this.outer) as |foo|~}}
General Kenobi!
{{~/let~}} - `, { foo, inner: 'x', outer: 'y', cond: true }); + `, + { foo, inner: 'x', outer: 'y', cond: true } + ); this.assertHTML('
General Kenobi!
'); this.assertStableRerender(); @@ -84,7 +85,6 @@ class DynamicModifiersResolutionModeTest extends RenderTest { this.rerender({ cond: false }); this.rerender({ cond: true }); assert.deepEqual(receivedArgs, ['y', 'x']); - } @test diff --git a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts index bda4f7984f..ad43550b90 100644 --- a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts @@ -319,7 +319,7 @@ if (hasDom) { 'asserts when eventName is missing'(assert: Assert) { assert.throws(() => { this.render(``, { - callback() { }, + callback() {}, }); }, /You must pass a valid DOM event name as the first argument to the `on` modifier/u); } @@ -328,7 +328,7 @@ if (hasDom) { 'asserts when eventName is a bound undefined value'(assert: Assert) { assert.throws(() => { this.render(``, { - callback() { }, + callback() {}, }); }, /You must pass a valid DOM event name as the first argument to the `on` modifier/u); } @@ -337,7 +337,7 @@ if (hasDom) { 'asserts when eventName is a function'(assert: Assert) { assert.throws(() => { this.render(``, { - callback() { }, + callback() {}, }); }, /You must pass a valid DOM event name as the first argument to the `on` modifier/u); } @@ -384,7 +384,7 @@ if (hasDom) { 'asserts if more than 2 positional parameters are provided'(assert: Assert) { assert.throws(() => { this.render(``, { - callback() { }, + callback() {}, someArg: 'foo', }); }, /You can only pass two positional arguments \(event name and callback\) to the `on` modifier, but you provided 3. Consider using the `fn` helper to provide additional arguments to the `on` callback./u); diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts index 79633d5e8b..014907c7f5 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts @@ -196,8 +196,10 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => { if (manager === null) { if (import.meta.env.DEV) { throw new Error( - `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${ref.debugLabel - }}}\`, and the incorrect definition is the value at the path \`${ref.debugLabel + `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${ + ref.debugLabel + }}}\`, and the incorrect definition is the value at the path \`${ + ref.debugLabel }\`, which was: ${debugToString!(hostDefinition)}` ); } else { diff --git a/packages/@glimmer/runtime/lib/modifiers/on.ts b/packages/@glimmer/runtime/lib/modifiers/on.ts index 256dc6b5fb..fd6f0b3427 100644 --- a/packages/@glimmer/runtime/lib/modifiers/on.ts +++ b/packages/@glimmer/runtime/lib/modifiers/on.ts @@ -86,8 +86,9 @@ export class OnModifierState { valueForRef(userProvidedCallbackReference), CheckFunction, (actual) => { - return `You must pass a function as the second argument to the \`on\` modifier; you passed ${actual === null ? 'null' : typeof actual - }. While rendering:\n\n${userProvidedCallbackReference.debugLabel ?? `{unlabeled value}`}`; + return `You must pass a function as the second argument to the \`on\` modifier; you passed ${ + actual === null ? 'null' : typeof actual + }. While rendering:\n\n${userProvidedCallbackReference.debugLabel ?? `{unlabeled value}`}`; } ) as EventListener; @@ -106,11 +107,12 @@ export class OnModifierState { if (this.shouldUpdate) { if (needsCustomCallback) { - this.callback = function(this: Element, event) { + this.callback = function (this: Element, event) { if (import.meta.env.DEV && passive) { event.preventDefault = () => { throw new Error( - `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${userProvidedCallback.name ?? `{anonymous function}` + `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${ + userProvidedCallback.name ?? `{anonymous function}` }` ); }; From 165dd49b7b71540487e3a0b4f9886594753d1ef3 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 23 Feb 2024 16:08:02 -0500 Subject: [PATCH 5/8] whoops lol --- .../@glimmer/runtime/lib/compiled/opcodes/dom.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts index 014907c7f5..2000caf45c 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts @@ -22,7 +22,7 @@ import { import { associateDestroyableChild, destroy } from '@glimmer/destroyable'; import { getInternalModifierManager } from '@glimmer/manager'; import { createComputeRef, isConstRef, valueForRef } from '@glimmer/reference'; -import { debugToString, expect, isObject } from '@glimmer/util'; +import { assign, debugToString, expect, isObject } from '@glimmer/util'; import { consumeTag, CURRENT_TAG, validateTag, valueForTag } from '@glimmer/validator'; import { $t0, CurriedTypes, Op } from '@glimmer/vm'; @@ -180,11 +180,11 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => { owner = curriedOwner; if (positional !== undefined) { - args.positional = [...positional, ...outerPositional] as CapturedPositionalArguments; + args.positional = positional.concat(outerPositional) as CapturedPositionalArguments; } if (named !== undefined) { - args.named = Object.assign({}, named, outerNamed); + args.named = assign({}, ...named, outerNamed); } } else { hostDefinition = value; @@ -196,10 +196,8 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => { if (manager === null) { if (import.meta.env.DEV) { throw new Error( - `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${ - ref.debugLabel - }}}\`, and the incorrect definition is the value at the path \`${ - ref.debugLabel + `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${ref.debugLabel + }}}\`, and the incorrect definition is the value at the path \`${ref.debugLabel }\`, which was: ${debugToString!(hostDefinition)}` ); } else { From f3f5f675f9a356008b317993a9a1da5a94a0b25d Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 23 Feb 2024 16:08:24 -0500 Subject: [PATCH 6/8] re-get rid of assign --- packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts index 2000caf45c..9b2e2f5a75 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts @@ -22,7 +22,7 @@ import { import { associateDestroyableChild, destroy } from '@glimmer/destroyable'; import { getInternalModifierManager } from '@glimmer/manager'; import { createComputeRef, isConstRef, valueForRef } from '@glimmer/reference'; -import { assign, debugToString, expect, isObject } from '@glimmer/util'; +import { debugToString, expect, isObject } from '@glimmer/util'; import { consumeTag, CURRENT_TAG, validateTag, valueForTag } from '@glimmer/validator'; import { $t0, CurriedTypes, Op } from '@glimmer/vm'; @@ -184,7 +184,7 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => { } if (named !== undefined) { - args.named = assign({}, ...named, outerNamed); + args.named = Object.assign({}, ...named, outerNamed); } } else { hostDefinition = value; From 30d3e0bb06ba8b2aa7f8202af120949af567404e Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 23 Feb 2024 16:20:31 -0500 Subject: [PATCH 7/8] lint:fix --- packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts index 9b2e2f5a75..d6c2a377dc 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts @@ -196,8 +196,10 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => { if (manager === null) { if (import.meta.env.DEV) { throw new Error( - `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${ref.debugLabel - }}}\`, and the incorrect definition is the value at the path \`${ref.debugLabel + `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${ + ref.debugLabel + }}}\`, and the incorrect definition is the value at the path \`${ + ref.debugLabel }\`, which was: ${debugToString!(hostDefinition)}` ); } else { From fdcce4b723495b5b8365207f21e3e860bb0e3849 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 23 Feb 2024 16:27:13 -0500 Subject: [PATCH 8/8] types --- .../integration-tests/test/modifiers/dynamic-modifiers-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts b/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts index 66c45c5448..87f6f5b9a8 100644 --- a/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts @@ -66,7 +66,7 @@ class DynamicModifiersResolutionModeTest extends RenderTest { } @test - 'Modifiers with dynamic arguments receive the correct number of arguments'(assert) { + 'Modifiers with dynamic arguments receive the correct number of arguments'(assert: Assert) { let receivedArgs: unknown[] = []; const foo = defineSimpleModifier((_element: unknown, args: unknown[]) => (receivedArgs = args));