diff --git a/packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js b/packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js index d5ca43618d0..0d6b9bdb58c 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js @@ -11,7 +11,6 @@ import { Component } from '@ember/-internals/glimmer'; import { setModifierManager, modifierCapabilities } from '@glimmer/manager'; import EmberObject, { set } from '@ember/object'; import { tracked } from '@ember/-internals/metal'; -import { backtrackingMessageFor } from '../utils/debug-stack'; class ModifierManagerTest extends RenderingTestCase { '@test throws a useful error when missing capabilities'(assert) { @@ -242,9 +241,9 @@ class ModifierManagerTest extends RenderingTestCase { assert.equal(updateCount, 2); } - '@test provides a helpful assertion when mutating a value that was consumed already'() { + '@test allows get/set (lazy initialization) within modifier didInsertElement'(assert) { class Person { - @tracked name = 'bob'; + @tracked name; } let ModifierClass = setModifierManager( @@ -262,24 +261,25 @@ class ModifierManagerTest extends RenderingTestCase { } ); + let person = new Person(); + this.registerModifier( 'foo-bar', class MyModifier extends ModifierClass { didInsertElement([person]) { - person.name; - person.name = 'sam'; + // get/set/get pattern (lazy initialization) + let val = person.name; + if (val === undefined) { + person.name = 'sam'; + val = person.name; + } } } ); - let expectedMessage = backtrackingMessageFor('name', Person.name, { - renderTree: [], - includeTopLevel: false, - }); + this.render('

hello world

', { person }); - expectAssertion(() => { - this.render('

hello world

', { person: new Person() }); - }, expectedMessage); + assert.strictEqual(person.name, 'sam', 'lazy initialization in modifier works'); } '@test capabilities helper function must be used to generate capabilities'(assert) { diff --git a/packages/@ember/-internals/glimmer/tests/integration/helpers/helper-manager-test.js b/packages/@ember/-internals/glimmer/tests/integration/helpers/helper-manager-test.js index c585cb9ef79..2f08125ea92 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/helpers/helper-manager-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/helpers/helper-manager-test.js @@ -6,7 +6,6 @@ import { set } from '@ember/object'; import { setOwner } from '@ember/-internals/owner'; import Service, { service } from '@ember/service'; import { registerDestructor } from '@glimmer/destroyable'; -import { backtrackingMessageFor } from '../../utils/debug-stack'; class TestHelperManager { capabilities = helperCapabilities('3.23', { @@ -216,7 +215,7 @@ moduleFor( this.assertText('hello'); } - ['@test debug name is used for backtracking message']() { + ['@test allows get/set within the same tracking frame (lazy initialization)']() { this.registerCustomHelper( 'hello', class extends TestHelper { @@ -225,17 +224,13 @@ moduleFor( value() { this.foo; this.foo = 456; + return this.foo; } } ); - let expectedMessage = backtrackingMessageFor('foo', '.*', { - renderTree: ['\\(result of a `TEST_HELPER` helper\\)'], - }); - - expectAssertion(() => { - this.render('{{hello}}'); - }, expectedMessage); + this.render('{{hello}}'); + this.assertText('456'); } ['@test asserts against using both `hasValue` and `hasScheduledEffect`'](assert) { @@ -347,20 +342,20 @@ moduleFor( assert.verifySteps([]); } - '@test custom helpers gives helpful assertion when reading then mutating a tracked value within constructor'() { + '@test custom helpers allows get/set/get (lazy initialization) within constructor'() { this.registerCustomHelper( 'hello', class extends TestHelper { - @tracked foo = 123; + @tracked foo; constructor() { super(...arguments); - // first read the tracked property - this.foo; - - // then attempt to update the tracked property - this.foo = 456; + // get/set/get pattern (lazy initialization) + let val = this.foo; + if (val === undefined) { + this.foo = 456; + } } value() { @@ -369,36 +364,29 @@ moduleFor( } ); - let expectedMessage = backtrackingMessageFor('foo'); - - expectAssertion(() => { - this.render('{{hello}}'); - }, expectedMessage); + this.render('{{hello}}'); + this.assertText('456'); } - '@test custom helpers gives helpful assertion when reading then mutating a tracked value within value'() { + '@test custom helpers allows get/set/get (lazy initialization) within value'() { this.registerCustomHelper( 'hello', class extends TestHelper { - @tracked foo = 123; + @tracked foo; value() { - // first read the tracked property - this.foo; - - // then attempt to update the tracked property - this.foo = 456; + // get/set/get pattern (lazy initialization) + let val = this.foo; + if (val === undefined) { + this.foo = 456; + } + return this.foo; } } ); - let expectedMessage = backtrackingMessageFor('foo', '.*', { - renderTree: ['\\(result of a `TEST_HELPER` helper\\)'], - }); - - expectAssertion(() => { - this.render('{{hello}}'); - }, expectedMessage); + this.render('{{hello}}'); + this.assertText('456'); } } ); diff --git a/packages/@ember/-internals/metal/tests/tracked/validation_test.js b/packages/@ember/-internals/metal/tests/tracked/validation_test.js index 307c35dcc4e..ccb144fe267 100644 --- a/packages/@ember/-internals/metal/tests/tracked/validation_test.js +++ b/packages/@ember/-internals/metal/tests/tracked/validation_test.js @@ -363,7 +363,24 @@ moduleFor( ); } - ['@test gives helpful assertion when a tracked property is mutated after access in with an autotracking transaction']() { + ['@test allows get/set/get (lazy initialization) within the same tracking frame'](assert) { + class MyObject { + @tracked value; + toString() { + return 'MyObject'; + } + } + + let obj = new MyObject(); + + track(() => { + obj.value; + obj.value = 123; + assert.strictEqual(obj.value, 123, 'get after set returns the updated value'); + }); + } + + ['@test gives helpful assertion when a tracked property is mutated after access without re-read']() { class MyObject { @tracked value; toString() { diff --git a/packages/@glimmer-workspace/integration-tests/test/managers/helper-manager-test.ts b/packages/@glimmer-workspace/integration-tests/test/managers/helper-manager-test.ts index 839d7295024..ad63441bfe7 100644 --- a/packages/@glimmer-workspace/integration-tests/test/managers/helper-manager-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/managers/helper-manager-test.ts @@ -352,19 +352,19 @@ class HelperManagerTest extends RenderTest { this.assertHTML('hello'); } - @test({ skip: !DEBUG }) 'debug name is used for backtracking message'(assert: Assert) { + @test({ skip: !DEBUG }) 'allows get/set within the same tracking frame (lazy initialization)'() { class Hello extends TestHelper { @tracked foo = 123; value() { consume(this.foo); this.foo = 456; + return this.foo; } } - assert.throws(() => { - this.renderComponent(defineComponent({ hello: Hello }, '{{hello}}')); - }, /You attempted to update `foo` on/u); + this.renderComponent(defineComponent({ hello: Hello }, '{{hello}}')); + this.assertHTML('456'); } @test({ skip: !DEBUG }) 'asserts against using both `hasValue` and `hasScheduledEffect`'( @@ -451,21 +451,18 @@ class HelperManagerTest extends RenderTest { } @test({ skip: !DEBUG }) - 'custom helpers gives helpful assertion when reading then mutating a tracked value within constructor'( - assert: Assert - ) { + 'custom helpers allows get/set/get (lazy initialization) within constructor'() { class Hello extends TestHelper { - @tracked foo = 123; + @tracked foo: number | undefined; constructor(owner: Owner, args: Arguments) { super(owner, args); - // first read the tracked property - - consume(this.foo); - - // then attempt to update the tracked property - this.foo = 456; + // get/set/get pattern (lazy initialization) + let val = this.foo; + if (val === undefined) { + this.foo = 456; + } } value() { @@ -473,31 +470,27 @@ class HelperManagerTest extends RenderTest { } } - assert.throws(() => { - this.renderComponent(defineComponent({ hello: Hello }, '{{hello}}')); - }, /You attempted to update `foo` on /u); + this.renderComponent(defineComponent({ hello: Hello }, '{{hello}}')); + this.assertHTML('456'); } @test({ skip: !DEBUG }) - 'custom helpers gives helpful assertion when reading then mutating a tracked value within value'( - assert: Assert - ) { + 'custom helpers allows get/set/get (lazy initialization) within value'() { class Hello extends TestHelper { - @tracked foo = 123; + @tracked foo: number | undefined; value() { - // first read the tracked property - - consume(this.foo); - - // then attempt to update the tracked property - this.foo = 456; + // get/set/get pattern (lazy initialization) + let val = this.foo; + if (val === undefined) { + this.foo = 456; + } + return this.foo; } } - assert.throws(() => { - this.renderComponent(defineComponent({ hello: Hello }, '{{hello}}')); - }, /You attempted to update `foo` on /u); + this.renderComponent(defineComponent({ hello: Hello }, '{{hello}}')); + this.assertHTML('456'); } } diff --git a/packages/@glimmer-workspace/integration-tests/test/managers/modifier-manager-test.ts b/packages/@glimmer-workspace/integration-tests/test/managers/modifier-manager-test.ts index dd93d05ed05..973426a96da 100644 --- a/packages/@glimmer-workspace/integration-tests/test/managers/modifier-manager-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/managers/modifier-manager-test.ts @@ -217,21 +217,19 @@ abstract class ModifierManagerTest extends RenderTest { } @test({ skip: !DEBUG }) - 'provides a helpful deprecation when mutating a tracked value that was consumed already within constructor'( - assert: Assert - ) { + 'allows get/set (lazy initialization) within modifier constructor'() { class Foo extends CustomModifier { - @tracked foo = 123; + @tracked foo: number | undefined; constructor(owner: Owner, args: Arguments) { super(owner, args); - // first read the tracked property - - consume(this.foo); - - // then attempt to update the tracked property - this.foo = 456; + // get/set/get pattern (lazy initialization) + let val = this.foo; + if (val === undefined) { + this.foo = 456; + val = this.foo; + } } override didInsertElement() {} @@ -243,9 +241,8 @@ abstract class ModifierManagerTest extends RenderTest { let Main = defineComponent({ foo }, '

hello world

'); - assert.throws(() => { - this.renderComponent(Main); - }, /You attempted to update `foo` on `.*`, but it had already been used previously in the same computation/u); + this.renderComponent(Main); + this.assertHTML('

hello world

'); } @test diff --git a/packages/@glimmer/validator/lib/debug.ts b/packages/@glimmer/validator/lib/debug.ts index 3578a060562..579591b1dc5 100644 --- a/packages/@glimmer/validator/lib/debug.ts +++ b/packages/@glimmer/validator/lib/debug.ts @@ -34,6 +34,14 @@ interface Transaction { if (DEBUG) { let CONSUMED_TAGS: WeakMap | null = null; + // Tracks tags that were dirtied after being consumed within the same tracking frame. + // If a tag is re-consumed before the frame ends, it's removed (get/set/get is OK). + // Any remaining entries at end-of-frame trigger the backtracking assertion. + let PENDING_SAME_FRAME_ASSERTIONS: Map< + Tag, + { obj?: unknown; keyName?: PropertyKey; transaction: Transaction } + > | null = null; + const TRANSACTION_STACK: Transaction[] = []; ///////// @@ -81,10 +89,29 @@ if (DEBUG) { throw new Error('attempted to close a tracking transaction, but one was not open'); } - TRANSACTION_STACK.pop(); + let closingTransaction = TRANSACTION_STACK.pop(); + + // Check for any same-frame get/set that was never followed by a re-get. + // This catches patterns like `get → set` (without a second get) which can + // cause infinite revalidation loops. + if (PENDING_SAME_FRAME_ASSERTIONS !== null && closingTransaction !== undefined) { + for (let [, pending] of PENDING_SAME_FRAME_ASSERTIONS) { + if (pending.transaction === closingTransaction) { + // This tag was dirtied after being consumed but never re-consumed + // before the frame ended — this is likely a bug, not lazy init. + let message = TRANSACTION_ENV.debugMessage( + pending.obj, + pending.keyName != null ? String(pending.keyName) : undefined + ); + PENDING_SAME_FRAME_ASSERTIONS = null; + assert(false, message); + } + } + } if (TRANSACTION_STACK.length === 0) { CONSUMED_TAGS = null; + PENDING_SAME_FRAME_ASSERTIONS = null; } }; @@ -97,6 +124,7 @@ if (DEBUG) { TRANSACTION_STACK.splice(0, TRANSACTION_STACK.length); CONSUMED_TAGS = null; + PENDING_SAME_FRAME_ASSERTIONS = null; return stack; }; @@ -178,7 +206,15 @@ if (DEBUG) { }; debug.markTagAsConsumed = (_tag: Tag) => { - if (!CONSUMED_TAGS || CONSUMED_TAGS.has(_tag)) return; + if (!CONSUMED_TAGS) return; + + // If this tag has a pending same-frame assertion, the re-consume (second get) + // resolves it — this is the valid get/set/get (lazy initialization) pattern. + if (PENDING_SAME_FRAME_ASSERTIONS !== null && PENDING_SAME_FRAME_ASSERTIONS.has(_tag)) { + PENDING_SAME_FRAME_ASSERTIONS.delete(_tag); + } + + if (CONSUMED_TAGS.has(_tag)) return; CONSUMED_TAGS.set(_tag, getLast(asPresentArray(TRANSACTION_STACK))); @@ -203,6 +239,20 @@ if (DEBUG) { if (!transaction) return; + // Allow get/set/get (lazy initialization) within the same tracking frame. + // Instead of throwing immediately, defer the assertion to end-of-frame. + // If the tag is re-consumed (second get) before the frame ends, the pending + // assertion is cleared. Otherwise it fires at endTrackingTransaction. + let currentTransaction = TRANSACTION_STACK[TRANSACTION_STACK.length - 1]; + if (transaction === currentTransaction) { + if (PENDING_SAME_FRAME_ASSERTIONS === null) { + PENDING_SAME_FRAME_ASSERTIONS = new Map(); + } + PENDING_SAME_FRAME_ASSERTIONS.set(tag, { obj, keyName, transaction }); + CONSUMED_TAGS.delete(tag); + return; + } + // This hack makes the assertion message nicer, we can cut off the first // few lines of the stack trace and let users know where the actual error // occurred. diff --git a/packages/@glimmer/validator/test/tracking-test.ts b/packages/@glimmer/validator/test/tracking-test.ts index 1a0fc4c1dc0..2a799723810 100644 --- a/packages/@glimmer/validator/test/tracking-test.ts +++ b/packages/@glimmer/validator/test/tracking-test.ts @@ -486,7 +486,34 @@ module('@glimmer/validator: tracking', () => { }); if (DEBUG) { - test('it errors when attempting to update a value already consumed in the same transaction', (assert) => { + test('it allows get/set/get (lazy initialization) within the same tracking frame', (assert) => { + class Foo { + foo = 123; + bar = 456; + } + + let { getter, setter } = trackedData('foo', function (this: Foo) { + return this.bar; + }); + + let foo = new Foo(); + + let result: number | undefined; + + // get/set/get within the same tracking frame should not throw + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- @fixme + debug.runInTrackingTransaction!(() => { + track(() => { + getter(foo); + setter(foo, 789); + result = getter(foo); + }); + }); + + assert.strictEqual(result, 789, 'get after set returns the updated value'); + }); + + test('it errors when get/set is not followed by a re-get in the same frame', (assert) => { class Foo { foo = 123; bar = 456; @@ -508,12 +535,53 @@ module('@glimmer/validator: tracking', () => { }); }, /You attempted to update `foo` on `Foo`/); }); + + test('it still errors when updating a value consumed in a previous tracking frame', (assert) => { + class Foo { + foo = 123; + bar = 456; + } + + let { getter, setter } = trackedData('foo', function (this: Foo) { + return this.bar; + }); + + let foo = new Foo(); + + assert.throws(() => { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- @fixme + debug.runInTrackingTransaction!(() => { + track(() => { + getter(foo); + }); + + track(() => { + setter(foo, 789); + }); + }); + }, /You attempted to update `foo` on `Foo`/); + }); } }); if (DEBUG) { module('debug', () => { - test('it errors when attempting to update a value that has already been consumed in the same transaction', (assert) => { + test('it allows consume/dirty/consume (lazy initialization) within the same tracking frame', (assert) => { + assert.expect(0); + let tag = createTag(); + + // consume/dirty/consume within the same frame should not throw (get/set/get pattern) + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- @fixme + debug.runInTrackingTransaction!(() => { + track(() => { + consumeTag(tag); + dirtyTag(tag); + consumeTag(tag); + }); + }); + }); + + test('it errors when consume/dirty is not followed by a re-consume in the same frame', (assert) => { let tag = createTag(); assert.throws(() => { @@ -524,7 +592,7 @@ module('@glimmer/validator: tracking', () => { dirtyTag(tag); }); }); - }, /Error: Assertion Failed: You attempted to update `undefined`/u); + }, /Error: Assertion Failed: You attempted to update/u); }); test('it throws errors across track frames within the same debug transaction', (assert) => {