Skip to content

Commit

Permalink
[BUGFIX release] Use deprecation for mutation in constructor during r…
Browse files Browse the repository at this point in the history
…endering

During recent refactors that landed in glimmer-vm@0.57 (Glimmer VM
version that landed in Ember 3.22) we moved the internal infrastructure
to ensure **everything** was in a tracking frame. This greatly increased
the number of locations that would now fall under the standard read +
write assertions.

For example, in glimmer-vm@0.56 the following would run without error:

```js
import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';

export default class extends Component {
  @Tracked counter = 0;

    constructor() {
      super(...arguments);

      this.counter++;
    }
}
```

Now, obviously this is a bit contrived, but the point is that since
component construction was not previously within a tracking frame it
would **not** error due to the read then write of a tracked property.

As of glimmer-vm@0.60+ we **do** run this within a tracking frame
(because nearly everything is automatically within a tracking frame).

---

This change ensures that the (previously untracked) construction of
components, modifiers, and helpers trigger a deprecation **not** an
assertion when mutating a previously read value within the constructor.
  • Loading branch information
rwjblue committed Nov 20, 2020
1 parent 5da2bc7 commit 3a613d6
Show file tree
Hide file tree
Showing 9 changed files with 196 additions and 16 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Factory } from '@ember/-internals/owner';
import { assert } from '@ember/debug';
import { DEBUG } from '@glimmer/env';
import {
Arguments,
ComponentCapabilities,
Expand All @@ -24,6 +25,7 @@ import {
buildCapabilities,
registerDestructor,
} from '@glimmer/runtime';
import { deprecateMutationsInTrackingTransaction } from '@glimmer/validator';
import { argsProxyFor } from '../utils/args-proxy';

const CAPABILITIES = {
Expand Down Expand Up @@ -136,11 +138,19 @@ export default class CustomComponentManager<ComponentInstance>
let { delegate } = definition;
let args = argsProxyFor(vmArgs.capture(), 'component');

let component = delegate.createComponent(definition.ComponentClass.class!, args);
let component;

if (DEBUG && deprecateMutationsInTrackingTransaction !== undefined) {
deprecateMutationsInTrackingTransaction(() => {
component = delegate.createComponent(definition.ComponentClass.class!, args);
});
} else {
component = delegate.createComponent(definition.ComponentClass.class!, args);
}

let bucket = new CustomComponentState(delegate, component, args, env);

return bucket;
return bucket as CustomComponentState<ComponentInstance>;
}

getDebugName({ name }: CustomComponentDefinitionState<ComponentInstance>): string {
Expand Down
31 changes: 24 additions & 7 deletions packages/@ember/-internals/glimmer/lib/modifiers/custom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ import {
VMArguments,
} from '@glimmer/interfaces';
import { buildCapabilities, registerDestructor, reifyArgs } from '@glimmer/runtime';
import { createUpdatableTag, untrack, UpdatableTag } from '@glimmer/validator';
import {
createUpdatableTag,
deprecateMutationsInTrackingTransaction,
untrack,
UpdatableTag,
} from '@glimmer/validator';
import { SimpleElement } from '@simple-dom/interface';
import { argsProxyFor } from '../utils/args-proxy';

Expand Down Expand Up @@ -111,10 +116,22 @@ class InteractiveCustomModifierManager<ModifierInstance>
let { useArgsProxy, passFactoryToCreate } = delegate.capabilities;

let args = useArgsProxy ? argsProxyFor(capturedArgs, 'modifier') : reifyArgs(capturedArgs);
let instance = delegate.createModifier(
passFactoryToCreate ? ModifierClass : ModifierClass.class,
args
);

let instance: ModifierInstance;

if (DEBUG && deprecateMutationsInTrackingTransaction !== undefined) {
deprecateMutationsInTrackingTransaction(() => {
instance = delegate.createModifier(
passFactoryToCreate ? ModifierClass : ModifierClass.class,
args
);
});
} else {
instance = delegate.createModifier(
passFactoryToCreate ? ModifierClass : ModifierClass.class,
args
);
}

let tag = createUpdatableTag();
let state: CustomModifierState<ModifierInstance>;
Expand All @@ -124,14 +141,14 @@ class InteractiveCustomModifierManager<ModifierInstance>
element,
delegate,
args,
modifier: instance,
modifier: instance!,
};
} else {
state = {
tag,
element,
delegate,
modifier: instance,
modifier: instance!,
get args() {
return reifyArgs(capturedArgs);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3801,6 +3801,31 @@ moduleFor(

runTask(() => set(this.context, 'cond', false));
}

'@test tracked property mutation in init does not error'() {
// TODO: this should issue a deprecation, but since the curly manager
// uses an untracked frame for construction we don't (yet)
this.registerComponent('foo-bar', {
template: `{{this.itemCount}}`,
ComponentClass: class extends Component {
@tracked itemCount = 0;

init() {
super.init(...arguments);

// first read the tracked property
let { itemCount } = this;

// then attempt to update the tracked property
this.itemCount = itemCount + 1;
}
},
});

this.render('<FooBar />');

this.assertComponentElement(this.firstChild, { content: '1' });
}
}
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { DEBUG } from '@glimmer/env';
import { moduleFor, RenderingTestCase, runTask, strip } from 'internal-test-helpers';

import { Object as EmberObject } from '@ember/-internals/runtime';
import { set, setProperties, computed } from '@ember/-internals/metal';
import { set, setProperties, computed, tracked } from '@ember/-internals/metal';
import { setComponentManager, capabilities } from '@ember/-internals/glimmer';

const BasicComponentManager = EmberObject.extend({
Expand Down Expand Up @@ -897,5 +897,35 @@ moduleFor(

assert.verifySteps([]);
}

'@test tracked property mutation in constructor issues a deprecation'() {
let ComponentClass = setComponentManager(
createBasicManager,
class extends EmberObject {
@tracked itemCount = 0;

init() {
super.init(...arguments);

// first read the tracked property
let { itemCount } = this;

// then attempt to update the tracked property
this.itemCount = itemCount + 1;
}
}
);

this.registerComponent('foo-bar', {
template: `{{this.itemCount}}`,
ComponentClass,
});

expectDeprecation(() => {
this.render('<FooBar />');
}, /You attempted to update `itemCount` on `<.*>`, but it had already been used previously in the same computation/);

this.assertHTML(`1`);
}
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,50 @@ class ModifierManagerTest extends RenderingTestCase {
assert.equal(updateCount, 2);
}

'@test provides a helpful deprecation when mutating a tracked value that was consumed already within constructor'(
assert
) {
let ModifierClass = setModifierManager(
(owner) => {
return new this.CustomModifierManager(owner);
},
class {
static create() {
return new this();
}

@tracked foo = 123;

constructor() {
// first read the tracked property
this.foo;

// then attempt to update the tracked property
this.foo = 456;
}

didInsertElement() {}
didUpdate() {}
willDestroyElement() {}
}
);

this.registerModifier(
'foo-bar',
class extends ModifierClass {
didInsertElement() {
assert.ok(true, 'modifiers didInsertElement was called');
}
}
);

let expectedMessage = backtrackingMessageFor('foo');

expectDeprecation(() => {
this.render('<h1 {{foo-bar}}>hello world</h1>');
}, expectedMessage);
}

'@test provides a helpful assertion when mutating a value that was consumed already'() {
class Person {
@tracked name = 'bob';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ moduleFor(
}, expectedMessage);
}

['@test class-based helper gives helpful assertion when mutating a tracked property that was tracked already']() {
['@test class-based helper gives helpful deprecation when mutating a tracked property that was tracked already']() {
this.add(
'helper:hello-world',
class HelloWorld extends Helper {
Expand All @@ -746,7 +746,7 @@ moduleFor(
renderTree: ['\\(result of a `<HelloWorld.*?>` helper\\)'],
});

expectAssertion(() => {
expectDeprecation(() => {
this.render('{{hello-world}}');
}, expectedMessage);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,60 @@ if (EMBER_GLIMMER_HELPER_MANAGER) {

assert.verifySteps([]);
}

'@test custom helpers gives helpful assertion when reading then mutating a tracked value within constructor'() {
this.registerCustomHelper(
'hello',
class extends TestHelper {
@tracked foo = 123;

constructor() {
super(...arguments);

// first read the tracked property
this.foo;

// then attempt to update the tracked property
this.foo = 456;
}

value() {
return this.foo;
}
}
);

let expectedMessage = backtrackingMessageFor('foo');

expectAssertion(() => {
this.render('{{hello}}');
}, expectedMessage);
}

'@test custom helpers gives helpful assertion when reading then mutating a tracked value within value'() {
this.registerCustomHelper(
'hello',
class extends TestHelper {
@tracked foo = 123;

value() {
// first read the tracked property
this.foo;

// then attempt to update the tracked property
this.foo = 456;
}
}
);

let expectedMessage = backtrackingMessageFor('foo', '.*', {
renderTree: ['\\(result of a `TEST_HELPER` helper\\)'],
});

expectAssertion(() => {
this.render('{{hello}}');
}, expectedMessage);
}
}
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ moduleFor(
}, expectedMessage);
}

'@test simple helper gives helpful assertion when mutating a tracked property that was tracked already'() {
'@test simple helper gives helpful deprecation when mutating a tracked property that was tracked already'() {
class Person {
@tracked name = 'bob';
}
Expand All @@ -443,7 +443,7 @@ moduleFor(
renderTree: ['\\(result of a `\\(unknown function\\)` helper\\)'],
});

expectAssertion(() => {
expectDeprecation(() => {
this.render('{{hello-world this.model}}', { model: new Person() });
}, expectedMessage);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ moduleFor(
}, /You attempted to update `value` on `EmberObject`, but it had already been used previously in the same computation/);
}

['@test gives helpful assertion when a tracked property is mutated after access within unknownProperty within an autotracking transaction']() {
['@test gives helpful deprecation when a tracked property is mutated after access within unknownProperty within an autotracking transaction']() {
class EmberObject {
@tracked foo;

Expand All @@ -390,7 +390,7 @@ moduleFor(

let obj = new EmberObject();

expectAssertion(() => {
expectDeprecation(() => {
track(() => {
get(obj, 'bar');
});
Expand Down

0 comments on commit 3a613d6

Please sign in to comment.