[BUGFIX beta] Autotrack Modifiers and Helpers

[pzuraq]

This PR adds autotracking for modifiers and class based helpers.
Modifiers can opt out of autotracking via a new capabilities flag on
modifier managers.

[rwjblue]

Looking good, I left some inline comments / suggestions.

I think we should also make non-class based helpers auto-track, can you add that here as well?

[rwjblue]

This is a breaking change, we have to allow previously supported values for this

[pzuraq]

Yeah, I thought this was a bit weird. This is inconsistent with the component manager capabilities API, which asserts it must be a particular version. Happy to make the change here, but maybe we should also make the change there as well?

[rwjblue]

@pzuraq - No the component manager one is correct, the issue here is that we:

1. don't require modifier managers to have specified capabilities (just a bug in initial implementation)
2. any that did specify capabilities could have passed any arbitrary thing for the first argument

We should fix this mistake by doing:

• Adding a deprecation when we encounter a manager without capabilities set
• Deprecate calling this method (capabilities) with a value other than 3.13 (for the new flag)

[rwjblue]

We need to allow whatever version introduced managers, and we should probably add a deprecation for custom managers not having their capabilities set (nearly all of the managers in existence that I can find do not set capabilities at all)

[rwjblue]

This name does not match up at all with "auto tracking" to me, "disable lifecycle tracking" in the abstract seems like it would be more about handling teardown?

Some suggestions:

• disableTracking
• disableTrackedPropertyTracking
• disableAutoTracking

[rwjblue]

I would think this should be guarded as well?

let { capabilities } = delegate;
let shouldAutoTrack = capabilities === undefined || capabilities.disableLifecycleTracking !== true;

let tracked;

if (shouldAutoTrack) {
  tracked = track(() => {
    delegate.installModifier(modifier, element, args.value());
  });
} else {
  delegate.installModifier(modifier, element, args.value());
}

if (tracked !== undefined) {
  update(tag, tracked);
}

[pzuraq]

Wrapping it in a track call prevents external tracks from watching the values. My logic was that if you disabled tracking, you probably don't want anything to be able to track these computes, so we should swallow any autotracking within and throw it away. We can use the untrack function for this though.

[rwjblue]

you probably don't want anything to be able to track these computes, so we should swallow any autotracking within and throw it away

Hmm, I see. I think both are valid paths forward, definitely up to you here. Though if you don't mind, could you add a small inline comment about throwing away the tracker in the case where you have explicitly opted out?

[rwjblue]

Can we pick a better name for these tracked variables? I find it conceptually confusing to have a variable named tracked that is something different than @tracked. Maybe combinedTrackingTag or consumedTrackedPropertiesTag?

[pzuraq]

Yeahhhh, naming is hard lol, but yeah I'll go with the longer name

[pzuraq]

I think we should also make non-class based helpers auto-track, can you add that here as well?

We already had tests for those:

ember.js/packages/@ember/-internals/glimmer/tests/integration/helpers/tracked-test.js

Lines 12 to 214 in 98b4aa9

	'@test tracked properties rerender when updated'(assert) {
	let computeCount = 0;
	let PersonComponent = Component.extend({
	name: tracked({ value: 'bob' }),
	updateName() {
	this.name = 'sal';
	},
	});
	this.registerComponent('person', {
	ComponentClass: PersonComponent,
	template: strip`
	<button onclick={{action this.updateName}}>
	{{hello-world this.name}}
	</button>
	`,
	});
	this.registerHelper('hello-world', ([value]) => {
	computeCount++;
	return `${value}-value`;
	});
	this.render('<Person/>');
	this.assertText('bob-value');
	assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');
	runTask(() => this.rerender());
	this.assertText('bob-value');
	assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');
	runTask(() => this.$('button').click());
	this.assertText('sal-value');
	assert.strictEqual(computeCount, 2, 'compute is called exactly 2 times');
	}
	'@test nested tracked properties rerender when updated'(assert) {
	let computeCount = 0;
	let Person = EmberObject.extend({
	name: tracked({ value: 'bob' }),
	});
	this.registerHelper('hello-world', ([value]) => {
	computeCount++;
	return `${value}-value`;
	});
	this.render('{{hello-world model.name}}', {
	model: Person.create(),
	});
	this.assertText('bob-value');
	assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');
	runTask(() => this.rerender());
	this.assertText('bob-value');
	assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');
	runTask(() => (this.context.model.name = 'sal'));
	this.assertText('sal-value');
	assert.strictEqual(computeCount, 2, 'compute is called exactly 2 times');
	}
	'@test getters update when dependent properties are invalidated'(assert) {
	let computeCount = 0;
	let PersonComponent = Component.extend({
	first: tracked({ value: 'Rob' }),
	last: tracked({ value: 'Jackson' }),
	full: descriptor({
	get() {
	return `${this.first} ${this.last}`;
	},
	}),
	updatePerson() {
	this.first = 'Kris';
	this.last = 'Selden';
	},
	});
	this.registerComponent('person', {
	ComponentClass: PersonComponent,
	template: strip`
	<button onclick={{action this.updatePerson}}>
	{{hello-world this.full}}
	</button>
	`,
	});
	this.registerHelper('hello-world', ([value]) => {
	computeCount++;
	return value;
	});
	this.render('<Person/>');
	this.assertText('Rob Jackson');
	assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');
	runTask(() => this.rerender());
	this.assertText('Rob Jackson');
	assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');
	runTask(() => this.$('button').click());
	this.assertText('Kris Selden');
	assert.strictEqual(computeCount, 2, 'compute is called exactly 2 times');
	}
	'@test array properties rerender when updated'() {
	let NumListComponent = Component.extend({
	numbers: tracked({ initializer: () => A([1, 2, 3]) }),
	addNumber() {
	this.numbers.pushObject(4);
	},
	});
	this.registerComponent('num-list', {
	ComponentClass: NumListComponent,
	template: strip`
	<button {{action this.addNumber}}>
	{{join this.numbers}}
	</button>
	`,
	});
	this.registerHelper('join', ([value]) => {
	return value.join(', ');
	});
	this.render('<NumList />');
	this.assertText('1, 2, 3');
	runTask(() => this.$('button').click());
	this.assertText('1, 2, 3, 4');
	}
	'@test nested getters update when dependent properties are invalidated'(assert) {
	let computeCount = 0;
	let Person = EmberObject.extend({
	first: tracked({ value: 'Rob' }),
	last: tracked({ value: 'Jackson' }),
	full: descriptor({
	get() {
	return `${this.first} ${this.last}`;
	},
	}),
	});
	this.registerHelper('hello-world', ([value]) => {
	computeCount++;
	return value;
	});
	this.render('{{hello-world model.full}}', {
	model: Person.create(),
	});
	this.assertText('Rob Jackson');
	assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');
	runTask(() => this.rerender());
	this.assertText('Rob Jackson');
	assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');
	runTask(() => {
	this.context.model.first = 'Kris';
	this.context.model.last = 'Selden';
	});
	this.assertText('Kris Selden');
	assert.strictEqual(computeCount, 2, 'compute is called exactly 2 times');
	}

[rwjblue]

We already had tests for those:

Hmm, maybe I am mis-reading those but they do not look like what I was thinking. I mean specifically a test where a non-class based helper accesses a tracked value within its computation function. All of the tests that you linked seemed to be asserting that the helper will be reevaluated if an input to it was derived from a tracked property.

      '@test asdfasdfasdfasdfasdfasdfasdf'(assert) {
        let computeCount = 0;

		let currentUserService;
        this.registerService('current-user', Service.extend({
          name: tracked({ value: 'bob' }),

          init() {
            this._super(...arguments);
            currentUserService = this;
          }
        });

        this.registerComponent('person', {
          ComponentClass: Component.extend({
            currentUser: inject('current-user')
          }),

          template: strip`
            {{hello-world this.currentUser}}
          `,
        });

        this.registerHelper('hello-world', ([service]) => {
          computeCount++;
          return service.name;
        });

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

        this.assertText('bob-value');

        assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');

        runTask(() => this.rerender());

        this.assertText('bob-value');

        assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');

        runTask(() => currentUserService.name = 'sal');

        this.assertText('sal-value');

        assert.strictEqual(computeCount, 2, 'compute is called exactly 2 times');
      }

[pzuraq]

ah, gotcha, yeah that makes sense to add, will do 👍

[pzuraq]

Alright, updated based on feedback, plus I added ember-cli/ember-rfc176-data#166 to actually expose the modifier manager capabilities generator