Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX beta] Autotrack Modifiers and Helpers #18266

Merged
merged 1 commit into from
Aug 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@ember/-internals/glimmer/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,6 @@ export { default as OutletView } from './lib/views/outlet';
export { capabilities } from './lib/component-managers/custom';
export { setComponentManager, getComponentManager } from './lib/utils/custom-component-manager';
export { setModifierManager, getModifierManager } from './lib/utils/custom-modifier-manager';
export { capabilities as modifierCapabilties } from './lib/modifiers/custom';
export { capabilities as modifierCapabilities } from './lib/modifiers/custom';
export { isSerializationFirstNode } from './lib/utils/serialization-first-node-helpers';
export { setComponentTemplate, getComponentTemplate } from './lib/utils/component-template';
90 changes: 74 additions & 16 deletions packages/@ember/-internals/glimmer/lib/modifiers/custom.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { track, untrack } from '@ember/-internals/metal';
import { Factory } from '@ember/-internals/owner';
import { assert, deprecate } from '@ember/debug';
import { Dict, Opaque, Simple } from '@glimmer/interfaces';
import { CONSTANT_TAG, Tag } from '@glimmer/reference';
import { combine, CONSTANT_TAG, createUpdatableTag, Tag, update } from '@glimmer/reference';
import { Arguments, CapturedArguments, ModifierManager } from '@glimmer/runtime';

export interface CustomModifierDefinitionState<ModifierInstance> {
Expand All @@ -9,11 +11,36 @@ export interface CustomModifierDefinitionState<ModifierInstance> {
delegate: ModifierManagerDelegate<ModifierInstance>;
}

export interface Capabilities {}
export interface OptionalCapabilities {
disableAutoTracking?: boolean;
}

export interface Capabilities {
disableAutoTracking: boolean;
}

export function capabilities(
managerAPI: string,
optionalFeatures: OptionalCapabilities = {}
): Capabilities {
if (managerAPI !== '3.13') {
managerAPI = '3.13';

deprecate(
'Modifier manager capabilities now require you to pass a valid version when being generated. Valid versions include: 3.13',
false,
{
until: '3.17.0',
id: 'implicit-modifier-manager-capabilities',
}
);
}

assert('Invalid modifier manager compatibility specified', managerAPI === '3.13');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)


// Currently there are no capabilities for modifiers
export function capabilities(_managerAPI: string, _optionalFeatures?: {}): Capabilities {
return {};
return {
disableAutoTracking: Boolean(optionalFeatures.disableAutoTracking),
};
}

export class CustomModifierDefinition<ModifierInstance> {
Expand All @@ -39,6 +66,8 @@ export class CustomModifierDefinition<ModifierInstance> {
}

export class CustomModifierState<ModifierInstance> {
public tag = createUpdatableTag();

constructor(
public element: Simple.Element,
public delegate: ModifierManagerDelegate<ModifierInstance>,
Expand Down Expand Up @@ -101,26 +130,55 @@ class InteractiveCustomModifierManager<ModifierInstance>
definition: CustomModifierDefinitionState<ModifierInstance>,
args: Arguments
) {
let { delegate, ModifierClass } = definition;
const capturedArgs = args.capture();
let instance = definition.delegate.createModifier(
definition.ModifierClass,
capturedArgs.value()
);
return new CustomModifierState(element, definition.delegate, instance, capturedArgs);

let instance = definition.delegate.createModifier(ModifierClass, capturedArgs.value());

if (delegate.capabilities === undefined) {
delegate.capabilities = capabilities('3.13');

deprecate(
'Custom modifier managers must define their capabilities using the capabilities() helper function',
false,
{
until: '3.17.0',
id: 'implicit-modifier-manager-capabilities',
}
);
}

return new CustomModifierState(element, delegate, instance, capturedArgs);
}

getTag({ args }: CustomModifierState<ModifierInstance>): Tag {
return args.tag;
getTag({ args, tag }: CustomModifierState<ModifierInstance>): Tag {
return combine([tag, args.tag]);
}

install(state: CustomModifierState<ModifierInstance>) {
let { element, args, delegate, modifier } = state;
delegate.installModifier(modifier, element, args.value());
let { element, args, delegate, modifier, tag } = state;
let { capabilities } = delegate;

if (capabilities.disableAutoTracking === true) {
untrack(() => delegate.installModifier(modifier, element, args.value()));
} else {
let combinedTrackingTag = track(() =>
delegate.installModifier(modifier, element, args.value())
);
update(tag, combinedTrackingTag);
}
}

update(state: CustomModifierState<ModifierInstance>) {
let { args, delegate, modifier } = state;
delegate.updateModifier(modifier, args.value());
let { args, delegate, modifier, tag } = state;
let { capabilities } = delegate;

if (capabilities.disableAutoTracking === true) {
untrack(() => delegate.updateModifier(modifier, args.value()));
} else {
let combinedTrackingTag = track(() => delegate.updateModifier(modifier, args.value()));
update(tag, combinedTrackingTag);
}
}

getDestructor(state: CustomModifierState<ModifierInstance>) {
Expand Down
28 changes: 24 additions & 4 deletions packages/@ember/-internals/glimmer/lib/utils/references.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,16 +349,20 @@ export class SimpleHelperReference extends CachedReference {
}
}

private computeTag: UpdatableTag;
public tag: Tag;

constructor(private helper: HelperFunction, private args: CapturedArguments) {
super();
this.tag = args.tag;

let computeTag = (this.computeTag = createUpdatableTag());
this.tag = combine([args.tag, computeTag]);
}

compute(): Opaque {
let {
helper,
computeTag,
args: { positional, named },
} = this;

Expand All @@ -370,7 +374,12 @@ export class SimpleHelperReference extends CachedReference {
debugFreeze(namedValue);
}

return helper(positionalValue, namedValue);
let computedValue;
let combinedTrackingTag = track(() => (computedValue = helper(positionalValue, namedValue)));

update(computeTag, combinedTrackingTag);

return computedValue;
}
}

Expand All @@ -379,16 +388,20 @@ export class ClassBasedHelperReference extends CachedReference {
return new ClassBasedHelperReference(instance, args);
}

private computeTag: UpdatableTag;
public tag: Tag;

constructor(private instance: HelperInstance, private args: CapturedArguments) {
super();
this.tag = combine([instance[RECOMPUTE_TAG], args.tag]);

let computeTag = (this.computeTag = createUpdatableTag());
this.tag = combine([instance[RECOMPUTE_TAG], args.tag, computeTag]);
}

compute(): Opaque {
let {
instance,
computeTag,
args: { positional, named },
} = this;

Expand All @@ -400,7 +413,14 @@ export class ClassBasedHelperReference extends CachedReference {
debugFreeze(namedValue);
}

return instance.compute(positionalValue, namedValue);
let computedValue;
let combinedTrackingTag = track(
() => (computedValue = instance.compute(positionalValue, namedValue))
);

update(computeTag, combinedTrackingTag);

return computedValue;
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { moduleFor, RenderingTestCase, strip, classes, runTask } from 'internal-test-helpers';
import { ENV } from '@ember/-internals/environment';
import { setModifierManager } from '@ember/-internals/glimmer';
import { setModifierManager, modifierCapabilities } from '@ember/-internals/glimmer';
import { Object as EmberObject } from '@ember/-internals/runtime';

import { set, setProperties } from '@ember/-internals/metal';
Expand All @@ -9,6 +9,7 @@ import { Component } from '../../utils/helpers';

class CustomModifierManager {
constructor(owner) {
this.capabilities = modifierCapabilities('3.13');
this.owner = owner;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { moduleFor, RenderingTestCase, runTask } from 'internal-test-helpers';

import { Object as EmberObject } from '@ember/-internals/runtime';
import { setModifierManager } from '@ember/-internals/glimmer';
import { set } from '@ember/-internals/metal';
import { setModifierManager, modifierCapabilities } from '@ember/-internals/glimmer';
import { set, tracked } from '@ember/-internals/metal';

class ModifierManagerTest extends RenderingTestCase {}

class CustomModifierManager {
constructor(owner) {
this.capabilities = modifierCapabilities('3.13');
this.owner = owner;
}

Expand Down Expand Up @@ -101,6 +102,49 @@ moduleFor(
runTask(() => set(this.context, 'truthy', true));
}

'@test requires modifier capabilities'() {
class WithoutCapabilities extends CustomModifierManager {
constructor(owner) {
super(owner);
this.capabilities = undefined;
}
}

let ModifierClass = setModifierManager(
owner => {
return new WithoutCapabilities(owner);
},
EmberObject.extend({
didInsertElement() {},
didUpdate() {},
willDestroyElement() {},
})
);

this.registerModifier(
'foo-bar',
ModifierClass.extend({
didUpdate() {},
didInsertElement() {},
willDestroyElement() {},
})
);

expectDeprecation(() => {
this.render('<h1 {{foo-bar truthy}}>hello world</h1>');
}, /Custom modifier managers must define their capabilities/);
}

'@test modifier capabilities require a version'() {
expectDeprecation(() => {
modifierCapabilities();
}, /Modifier manager capabilities now require you to pass a valid version when being generated/);

expectDeprecation(() => {
modifierCapabilities('aoeu');
}, /Modifier manager capabilities now require you to pass a valid version when being generated/);
}

'@test associates manager even through an inheritance structure'(assert) {
assert.expect(5);
let ModifierClass = setModifierManager(
Expand Down Expand Up @@ -176,6 +220,64 @@ moduleFor(

runTask(() => set(this.context, 'truthy', 'true'));
}

'@test lifecycle hooks are autotracked by default'(assert) {
let TrackedClass = EmberObject.extend({
count: tracked({ value: 0 }),
});

let trackedOne = TrackedClass.create();
let trackedTwo = TrackedClass.create();

let insertCount = 0;
let updateCount = 0;

let ModifierClass = setModifierManager(
owner => {
return new CustomModifierManager(owner);
},
EmberObject.extend({
didInsertElement() {},
didUpdate() {},
willDestroyElement() {},
})
);

this.registerModifier(
'foo-bar',
ModifierClass.extend({
didInsertElement() {
// track the count of the first item
trackedOne.count;
insertCount++;
},

didUpdate() {
// track the count of the second item
trackedTwo.count;
updateCount++;
},
})
);

this.render('<h1 {{foo-bar truthy}}>hello world</h1>');
this.assertHTML(`<h1>hello world</h1>`);

assert.equal(insertCount, 1);
assert.equal(updateCount, 0);

runTask(() => trackedTwo.count++);
assert.equal(updateCount, 0);

runTask(() => trackedOne.count++);
assert.equal(updateCount, 1);

runTask(() => trackedOne.count++);
assert.equal(updateCount, 1);

runTask(() => trackedTwo.count++);
assert.equal(updateCount, 2);
}
}
);

Expand Down
Loading