From e6af3910357b1bef10604f44540139fb365a457e Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Sat, 28 Aug 2021 21:52:04 -0500 Subject: [PATCH 1/2] [cleanup]: Remove mutation after consumption autotracking.mutation-after-consumption --- .../@glimmer/manager/lib/public/component.ts | 11 +-- .../@glimmer/manager/lib/public/modifier.ts | 17 +--- packages/@glimmer/validator/index.ts | 1 - packages/@glimmer/validator/lib/debug.ts | 66 ++++------------ .../@glimmer/validator/test/tracking-test.ts | 77 ------------------- 5 files changed, 20 insertions(+), 152 deletions(-) diff --git a/packages/@glimmer/manager/lib/public/component.ts b/packages/@glimmer/manager/lib/public/component.ts index 9f76d676db..444e58d950 100644 --- a/packages/@glimmer/manager/lib/public/component.ts +++ b/packages/@glimmer/manager/lib/public/component.ts @@ -18,7 +18,6 @@ import { } from '@glimmer/interfaces'; import { createConstRef, Reference } from '@glimmer/reference'; import { registerDestructor } from '@glimmer/destroyable'; -import { deprecateMutationsInTrackingTransaction } from '@glimmer/validator'; import { buildCapabilities, FROM_CAPABILITIES } from '../util/capabilities'; import { argsProxyFor } from '../util/args-proxy'; import { ManagerFactory } from './index'; @@ -142,15 +141,7 @@ export class CustomComponentManager let delegate = this.getDelegateFor(owner); let args = argsProxyFor(vmArgs.capture(), 'component'); - let component: ComponentInstance; - - if (DEBUG && deprecateMutationsInTrackingTransaction !== undefined) { - deprecateMutationsInTrackingTransaction(() => { - component = delegate.createComponent(definition, args); - }); - } else { - component = delegate.createComponent(definition, args); - } + let component: ComponentInstance = delegate.createComponent(definition, args); return new CustomComponentState(component!, delegate, args); } diff --git a/packages/@glimmer/manager/lib/public/modifier.ts b/packages/@glimmer/manager/lib/public/modifier.ts index 74a2f76e6c..2b8c197732 100644 --- a/packages/@glimmer/manager/lib/public/modifier.ts +++ b/packages/@glimmer/manager/lib/public/modifier.ts @@ -12,12 +12,7 @@ import { registerDestructor } from '@glimmer/destroyable'; import { setOwner } from '@glimmer/owner'; import { valueForRef } from '@glimmer/reference'; import { assign, castToBrowser, dict } from '@glimmer/util'; -import { - createUpdatableTag, - deprecateMutationsInTrackingTransaction, - untrack, - UpdatableTag, -} from '@glimmer/validator'; +import { createUpdatableTag, untrack, UpdatableTag } from '@glimmer/validator'; import { SimpleElement } from '@simple-dom/interface'; import { buildCapabilities, FROM_CAPABILITIES } from '../util/capabilities'; import { argsProxyFor } from '../util/args-proxy'; @@ -115,8 +110,6 @@ export class CustomModifierManager let argsProxy = argsProxyFor(capturedArgs, 'modifier'); let args = useArgsProxy ? argsProxy : reifyArgs(capturedArgs); - let instance: ModifierInstance; - let factoryOrDefinition = definition; if (passFactoryToCreate) { @@ -134,13 +127,7 @@ export class CustomModifierManager }; } - if (DEBUG && deprecateMutationsInTrackingTransaction !== undefined) { - deprecateMutationsInTrackingTransaction(() => { - instance = delegate.createModifier(factoryOrDefinition, args); - }); - } else { - instance = delegate.createModifier(factoryOrDefinition, args); - } + let instance: ModifierInstance = delegate.createModifier(factoryOrDefinition, args); let tag = createUpdatableTag(); let state: CustomModifierState; diff --git a/packages/@glimmer/validator/index.ts b/packages/@glimmer/validator/index.ts index 9b4f3e815f..30a2a759c1 100644 --- a/packages/@glimmer/validator/index.ts +++ b/packages/@glimmer/validator/index.ts @@ -67,5 +67,4 @@ export { runInTrackingTransaction, beginTrackingTransaction, endTrackingTransaction, - deprecateMutationsInTrackingTransaction, } from './lib/debug'; diff --git a/packages/@glimmer/validator/lib/debug.ts b/packages/@glimmer/validator/lib/debug.ts index d28dd1160e..3f7fcb6659 100644 --- a/packages/@glimmer/validator/lib/debug.ts +++ b/packages/@glimmer/validator/lib/debug.ts @@ -1,6 +1,6 @@ import { Tag } from './validators'; import { DEBUG } from '@glimmer/env'; -import { deprecate, assert } from '@glimmer/global-context'; +import { assert } from '@glimmer/global-context'; export let beginTrackingTransaction: | undefined @@ -9,7 +9,6 @@ export let endTrackingTransaction: undefined | (() => void); export let runInTrackingTransaction: | undefined | ((fn: () => T, debuggingContext?: string | false) => T); -export let deprecateMutationsInTrackingTransaction: undefined | ((fn: () => void) => void); export let resetTrackingTransaction: undefined | (() => string); export let setTrackingTransactionEnv: @@ -27,7 +26,6 @@ export let logTrackingStack: undefined | ((transaction?: Transaction) => string) interface Transaction { parent: Transaction | null; debugLabel?: string; - deprecate: boolean; } if (DEBUG) { @@ -61,7 +59,7 @@ if (DEBUG) { setTrackingTransactionEnv = (env) => Object.assign(TRANSACTION_ENV, env); - beginTrackingTransaction = (_debugLabel?: string | false, deprecate = false) => { + beginTrackingTransaction = (_debugLabel?: string | false) => { CONSUMED_TAGS = CONSUMED_TAGS || new WeakMap(); let debugLabel = _debugLabel || undefined; @@ -71,7 +69,6 @@ if (DEBUG) { TRANSACTION_STACK.push({ parent, debugLabel, - deprecate, }); }; @@ -125,27 +122,6 @@ if (DEBUG) { } }; - /** - * Switches to deprecating within an autotracking transaction, if one exists. - * If `runInAutotrackingTransaction` is called within the callback of this - * method, it switches back to throwing an error, allowing zebra-striping of - * the types of errors that are thrown. - * - * Does not start an autotracking transaction. - * - * NOTE: For Ember usage only, in general you should assert that these - * invariants are true. - */ - deprecateMutationsInTrackingTransaction = (fn: () => void, debugLabel?: string | false) => { - beginTrackingTransaction!(debugLabel, true); - - try { - fn(); - } finally { - endTrackingTransaction!(); - } - }; - let nthIndex = (str: string, pattern: string, n: number, startingPos = -1) => { let i = startingPos; @@ -218,31 +194,23 @@ if (DEBUG) { if (!transaction) return; - let currentTransaction = TRANSACTION_STACK[TRANSACTION_STACK.length - 1]; - - if (currentTransaction.deprecate) { - deprecate(makeTrackingErrorMessage(transaction, obj, keyName), false, { - id: 'autotracking.mutation-after-consumption', - }); - } else { - // 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. - try { - assert(false, makeTrackingErrorMessage(transaction, obj, keyName)); - } catch (e) { - if (e.stack) { - let updateStackBegin = e.stack.indexOf('Stack trace for the update:'); - - if (updateStackBegin !== -1) { - let start = nthIndex(e.stack, '\n', 1, updateStackBegin); - let end = nthIndex(e.stack, '\n', 4, updateStackBegin); - e.stack = e.stack.substr(0, start) + e.stack.substr(end); - } + // 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. + try { + assert(false, makeTrackingErrorMessage(transaction, obj, keyName)); + } catch (e) { + if (e.stack) { + let updateStackBegin = e.stack.indexOf('Stack trace for the update:'); + + if (updateStackBegin !== -1) { + let start = nthIndex(e.stack, '\n', 1, updateStackBegin); + let end = nthIndex(e.stack, '\n', 4, updateStackBegin); + e.stack = e.stack.substr(0, start) + e.stack.substr(end); } - - throw e; } + + throw e; } }; } diff --git a/packages/@glimmer/validator/test/tracking-test.ts b/packages/@glimmer/validator/test/tracking-test.ts index 4dd002d07d..1621de511f 100644 --- a/packages/@glimmer/validator/test/tracking-test.ts +++ b/packages/@glimmer/validator/test/tracking-test.ts @@ -7,12 +7,9 @@ import { createTag, beginTrackFrame, endTrackFrame, - deprecateMutationsInTrackingTransaction, dirtyTag, - dirtyTagFor, isTracking, runInTrackingTransaction, - tagFor, track, trackedData, untrack, @@ -495,32 +492,6 @@ module('@glimmer/validator: tracking', () => { }); }, /You attempted to update `foo` on `\(an instance of/); }); - - test('it can switches to warning/deprecations when attempting to update a value already consumed in the same transaction', (assert) => { - class Foo { - foo = 123; - bar = 456; - } - - let { getter, setter } = trackedData('foo', function (this: Foo) { - return this.bar; - }); - - let foo = new Foo(); - - runInTrackingTransaction!(() => { - track(() => { - deprecateMutationsInTrackingTransaction!(() => { - getter(foo); - setter(foo, 789); - }); - }); - }); - - assert.validateDeprecations( - /You attempted to update `foo` on `.*`, but it had already been used previously in the same computation/ - ); - }); } }); @@ -585,54 +556,6 @@ module('@glimmer/validator: tracking', () => { }); }, /Error: You attempted to update `\(an unknown tag\)`/); }); - - test('it can switch to warnings/deprecations', (assert) => { - let tag = createTag(); - - runInTrackingTransaction!(() => { - track(() => { - deprecateMutationsInTrackingTransaction!(() => { - consumeTag(tag); - dirtyTag(tag); - }); - }); - }); - - assert.validateDeprecations( - /You attempted to update `.*`, but it had already been used previously in the same computation./ - ); - }); - - test('it switches back to errors with nested track calls', (assert) => { - let tag = createTag(); - - assert.throws(() => { - runInTrackingTransaction!(() => { - deprecateMutationsInTrackingTransaction!(() => { - track(() => { - consumeTag(tag); - dirtyTag(tag); - }); - }); - }); - }, /Error: You attempted to update `\(an unknown tag\)`/); - }); - - test('it gets a better error message with tagFor', (assert) => { - class Foo {} - let foo = new Foo(); - - assert.throws(() => { - runInTrackingTransaction!(() => { - deprecateMutationsInTrackingTransaction!(() => { - track(() => { - consumeTag(tagFor(foo, 'bar')); - dirtyTagFor(foo, 'bar'); - }); - }); - }); - }, /Error: You attempted to update `bar` on `\(an instance of .*\)`/); - }); }); } }); From b07dbcf785e2b685241670b69b13c2e2fab18786 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Fri, 10 Sep 2021 22:08:56 -0500 Subject: [PATCH 2/2] will error now --- .../test/managers/modifier-manager-test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/@glimmer/integration-tests/test/managers/modifier-manager-test.ts b/packages/@glimmer/integration-tests/test/managers/modifier-manager-test.ts index 6338b46f73..9f8cb58747 100644 --- a/packages/@glimmer/integration-tests/test/managers/modifier-manager-test.ts +++ b/packages/@glimmer/integration-tests/test/managers/modifier-manager-test.ts @@ -228,11 +228,9 @@ abstract class ModifierManagerTest extends RenderTest { let Main = defineComponent({ foo }, '

hello world

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