Skip to content

Commit

Permalink
[FEATURE] Removes Chains in Tracked Properties Flag
Browse files Browse the repository at this point in the history 
This PR updates the tracked properties feature flag to fully remove
chains. This was primarily is response to core issues we found with
interop with computed properties, which required us to make computed
properties and observers more lazy in general.

At a high level, these are the major changes:

* The `getCurrentTracker`/`setCurrentTracker` system was a leaky
  abstraction, since errors could cause a child tracker to never reset.
  In order to make it more bulletproof, it has been changed to
  `track`/`consume`/`isTracking`. We wrap any change to the stack of
  trackers with a `try/catch` to make sure we always clean up in
  `track`. `isTracking` is used only when we don't want to eagerly
  create a tag if there is no tracking context.
* Observers have been made asynchronous. They now flush during specific
  phases of the runloop - just before render, and potentially after
  render if anything was scheduled (this will begin a new runloop).
  They poll now to check if any changes occured, rather than firing
  synchronously when the change occurs, which is how we made them work
  entirely using tags without chains. Observers inherited from
  EmberObject classes begin polling at the same time as
  `finalizeChains`.
* Computed properties have been updated to only use chains when
  checking for dirtiness. Since computed properties were lazy before,
  this isn't much of a change overall. Computed properties also no
  longer autotrack, unless they have been marked by a (currently
  private) `_auto()` modifier.
* Both observers and computeds accomplish this laziness by following and
  reading the tags of their dependencies _after calculation_. They
  entangle all dependencies at this point, _unless_ the dependency is an
  uncalculated computed property. If they encounter one of these, they
  setup _lazy chains_, which will be followed and updated the next time
  the computed property is calculated. This also makes aliases lazily
  observe.
* Computed properties have also been updated to install a native setter,
  per the track props update RFC.
* Query parameters use synchronous observers to update various things.
  They should really be refactored, but that's going to take a while.
  In the meantime, we flush the observers synchronously for them
  specifically.
* A `UNKNOWN_PROPERTY_TAG` system has been added _privately and
  internally only_. This system allows proxies to return a special tag
  that invalidates when their _content's_ properties change. This system
  could be made more public in the future, but it is purposefully
  private for the time being. It was necessary to match existing
  semantics in many tests.
* A new mandatory setter system has been added. This system is now
  one-way - once a value has been consumed, there is no way to remove
  the setter and "unconsume" it. This is the nature of tags being lazy
  and having no teardown.

There were a few additional changes that were required as well:

* `visit` and `transitionTo` for application tests had to be made
  async in order to work properly with observers. Most of the work
  occured in another PR, but some had to be finished up here.
* Many, many tests needed to be updated. Most of these were for the fact
  that observers are now async, and required us to wait on the runloop
  settling.
  • Loading branch information
Chris Garrett committed Apr 26, 2019
1 parent ffb453b commit 4cdbb8c
Show file tree
Hide file tree
Showing 91 changed files with 3,760 additions and 2,542 deletions.
37 changes: 22 additions & 15 deletions packages/@ember/-internals/extension-support/tests/data_adapter_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { run } from '@ember/runloop';
import { get, set, addObserver, removeObserver } from '@ember/-internals/metal';
import { Object as EmberObject, A as emberA } from '@ember/-internals/runtime';
import EmberDataAdapter from '../lib/data_adapter';
import { moduleFor, ApplicationTestCase } from 'internal-test-helpers';
import { moduleFor, ApplicationTestCase, runLoopSettled } from 'internal-test-helpers';

let adapter;
const Model = EmberObject.extend();
Expand Down Expand Up @@ -267,23 +267,30 @@ moduleFor(
);
this.add('model:post', PostClass);

return this.visit('/').then(() => {
adapter = this.applicationInstance.lookup('data-adapter:main');
let release;

function recordsAdded() {
set(post, 'title', 'Post Modified');
}
return this.visit('/')
.then(() => {
adapter = this.applicationInstance.lookup('data-adapter:main');

function recordsUpdated(records) {
updatesCalled++;
assert.equal(records[0].columnValues.title, 'Post Modified');
}
function recordsAdded() {
set(post, 'title', 'Post Modified');
}

let release = adapter.watchRecords('post', recordsAdded, recordsUpdated);
release();
set(post, 'title', 'New Title');
assert.equal(updatesCalled, 1, 'Release function removes observers');
});
function recordsUpdated(records) {
updatesCalled++;
assert.equal(records[0].columnValues.title, 'Post Modified');
}

release = adapter.watchRecords('post', recordsAdded, recordsUpdated);

return runLoopSettled();
})
.then(() => {
release();
set(post, 'title', 'New Title');
assert.equal(updatesCalled, 1, 'Release function removes observers');
});
}

['@test _nameToClass does not error when not found'](assert) {
Expand Down
14 changes: 3 additions & 11 deletions packages/@ember/-internals/glimmer/lib/component-managers/custom.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getCurrentTracker } from '@ember/-internals/metal';
import { consume } from '@ember/-internals/metal';
import { Factory } from '@ember/-internals/owner';
import { HAS_NATIVE_PROXY } from '@ember/-internals/utils';
import { OwnedTemplateMeta } from '@ember/-internals/views';
Expand Down Expand Up @@ -171,12 +171,8 @@ export default class CustomComponentManager<ComponentInstance>
get(_target, prop) {
assert('args can only be strings', typeof prop === 'string');

let tracker = getCurrentTracker();
let ref = capturedArgs.named.get(prop as string);

if (tracker) {
tracker.add(ref.tag);
}
consume(ref.tag);

return ref.value();
},
Expand All @@ -200,11 +196,7 @@ export default class CustomComponentManager<ComponentInstance>
Object.defineProperty(namedArgsProxy, name, {
get() {
let ref = capturedArgs.named.get(name);
let tracker = getCurrentTracker();

if (tracker) {
tracker.add(ref.tag);
}
consume(ref.tag);

return ref.value();
},
Expand Down
45 changes: 17 additions & 28 deletions packages/@ember/-internals/glimmer/lib/utils/references.ts
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import {
consume,
didRender,
get,
getCurrentTracker,
set,
setCurrentTracker,
tagFor,
tagForProperty,
Tracker,
track,
watchKey,
} from '@ember/-internals/metal';
import { isProxy, symbol } from '@ember/-internals/utils';
Expand Down Expand Up @@ -190,7 +189,7 @@ export class RootPropertyReference extends PropertyReference
this.tag = this.propertyTag;
}

if (DEBUG) {
if (DEBUG && !EMBER_METAL_TRACKED_PROPERTIES) {
watchKey(parentValue, propertyKey);
}
}
Expand All @@ -202,22 +201,17 @@ export class RootPropertyReference extends PropertyReference
(this.tag.inner as ITwoWayFlushDetectionTag).didCompute(parentValue);
}

let parent: Option<Tracker> = null;
let tracker: Option<Tracker> = null;

if (EMBER_METAL_TRACKED_PROPERTIES) {
parent = getCurrentTracker();
tracker = setCurrentTracker();
}

let ret = get(parentValue, propertyKey);
let ret;

if (EMBER_METAL_TRACKED_PROPERTIES) {
setCurrentTracker(parent);
let tag = tracker!.combine();
if (parent) parent.add(tag);
let tag = track(() => {
ret = get(parentValue, propertyKey);
});

consume(tag);
this.propertyTag.inner.update(tag);
} else {
ret = get(parentValue, propertyKey);
}

return ret;
Expand Down Expand Up @@ -262,31 +256,26 @@ export class NestedPropertyReference extends PropertyReference {
if ((parentValueType === 'object' && _parentValue !== null) || parentValueType === 'function') {
let parentValue = _parentValue as object;

if (DEBUG) {
if (DEBUG && !EMBER_METAL_TRACKED_PROPERTIES) {
watchKey(parentValue, propertyKey);
}

if (DEBUG) {
(this.tag.inner as ITwoWayFlushDetectionTag).didCompute(parentValue);
}

let parent: Option<Tracker> = null;
let tracker: Option<Tracker> = null;
let ret;

if (EMBER_METAL_TRACKED_PROPERTIES) {
parent = getCurrentTracker();
tracker = setCurrentTracker();
}
let tag = track(() => {
ret = get(parentValue, propertyKey);
});

let ret = get(parentValue, propertyKey);

if (EMBER_METAL_TRACKED_PROPERTIES) {
setCurrentTracker(parent!);
let tag = tracker!.combine();
if (parent) parent.add(tag);
consume(tag);

propertyTag.inner.update(tag);
} else {
ret = get(parentValue, propertyKey);
propertyTag.inner.update(tagForProperty(parentValue, propertyKey));
}

Expand Down
20 changes: 10 additions & 10 deletions packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -856,7 +856,7 @@ moduleFor(
});
}

['@test query params in customized controllerName have stickiness by default between model'](
async ['@test query params in customized controllerName have stickiness by default between model'](
assert
) {
assert.expect(2);
Expand All @@ -867,16 +867,16 @@ moduleFor(
this.register('template:author', compile(tmpl));
});

return this.visit('/blog/author/1?official=true').then(() => {
let suffix1 = '/blog/author/1?official=true';
let href1 = this.element.querySelector('.author-1').href;
let suffix1337 = '/blog/author/1337';
let href1337 = this.element.querySelector('.author-1337').href;
await this.visit('/blog/author/1?official=true');

// check if link ends with the suffix
assert.ok(this.stringsEndWith(href1, suffix1), `${href1} ends with ${suffix1}`);
assert.ok(this.stringsEndWith(href1337, suffix1337), `${href1337} ends with ${suffix1337}`);
});
let suffix1 = '/blog/author/1?official=true';
let href1 = this.element.querySelector('.author-1').href;
let suffix1337 = '/blog/author/1337';
let href1337 = this.element.querySelector('.author-1337').href;

// check if link ends with the suffix
assert.ok(this.stringsEndWith(href1, suffix1), `${href1} ends with ${suffix1}`);
assert.ok(this.stringsEndWith(href1337, suffix1337), `${href1337} ends with ${suffix1337}`);
}

['@test visit() routable engine which errors on init'](assert) {
Expand Down
15 changes: 13 additions & 2 deletions packages/@ember/-internals/glimmer/tests/integration/application/rendering-test.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { moduleFor, ApplicationTestCase, strip } from 'internal-test-helpers';
import {
moduleFor,
ApplicationTestCase,
strip,
runTask,
runLoopSettled,
} from 'internal-test-helpers';

import { ENV } from '@ember/-internals/environment';
import Controller from '@ember/controller';
Expand Down Expand Up @@ -499,7 +505,12 @@ moduleFor(

await this.visit('/');

assert.rejectsAssertion(this.visit('/routeWithError'), expectedBacktrackingMessage);
assert.throwsAssertion(
() => runTask(() => this.visit('/routeWithError')),
expectedBacktrackingMessage
);

await runLoopSettled();
}

['@test route templates with {{{undefined}}} [GH#14924] [GH#16172]']() {
Expand Down
48 changes: 17 additions & 31 deletions packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ import {
equalsElement,
styles,
runTask,
runLoopSettled,
} from 'internal-test-helpers';

import { run } from '@ember/runloop';
import { DEBUG } from '@glimmer/env';
import { alias, set, get, observer, on, computed } from '@ember/-internals/metal';
import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features';
import Service, { inject as injectService } from '@ember/service';
import { Object as EmberObject, A as emberA } from '@ember/-internals/runtime';
import { jQueryDisabled } from '@ember/-internals/views';
Expand Down Expand Up @@ -2526,15 +2528,11 @@ moduleFor(
);
}

["@test when a property is changed during children's rendering"](assert) {
let outer, middle;
["@test when a property is changed during children's rendering"]() {
let middle;

this.registerComponent('x-outer', {
ComponentClass: Component.extend({
init() {
this._super(...arguments);
outer = this;
},
value: 1,
}),
template: '{{#x-middle}}{{x-inner value=value}}{{/x-middle}}',
Expand All @@ -2554,35 +2552,17 @@ moduleFor(
this.registerComponent('x-inner', {
ComponentClass: Component.extend({
value: null,
pushDataUp: observer('value', function() {
didReceiveAttrs() {
middle.set('value', this.get('value'));
}),
},
}),
template: '<div id="inner-value">{{value}}</div>',
});

this.render('{{x-outer}}');

assert.equal(this.$('#inner-value').text(), '1', 'initial render of inner');
assert.equal(
this.$('#middle-value').text(),
'',
'initial render of middle (observers do not run during init)'
);

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

assert.equal(this.$('#inner-value').text(), '1', 'initial render of inner');
assert.equal(
this.$('#middle-value').text(),
'',
'initial render of middle (observers do not run during init)'
);

let expectedBacktrackingMessage = /modified "value" twice on <.+?> in a single render\. It was rendered in "component:x-middle" and modified in "component:x-inner"/;

expectAssertion(() => {
runTask(() => outer.set('value', 2));
this.render('{{x-outer}}');
}, expectedBacktrackingMessage);
}

Expand Down Expand Up @@ -2741,9 +2721,13 @@ moduleFor(
this.assertText('initial value - initial value');

if (DEBUG) {
let message = EMBER_METAL_TRACKED_PROPERTIES
? /You attempted to update .*, but it is being tracked by a tracking context/
: /You must use set\(\) to set the `bar` property \(of .+\) to `foo-bar`\./;

expectAssertion(() => {
component.bar = 'foo-bar';
}, /You must use set\(\) to set the `bar` property \(of .+\) to `foo-bar`\./);
}, message);

this.assertText('initial value - initial value');
}
Expand Down Expand Up @@ -3208,7 +3192,7 @@ moduleFor(
this.assertText('things');
}

['@test didReceiveAttrs fires after .init() but before observers become active'](assert) {
async ['@test didReceiveAttrs fires after .init() but before observers become active'](assert) {
let barCopyDidChangeCount = 0;

this.registerComponent('foo-bar', {
Expand All @@ -3231,13 +3215,15 @@ moduleFor(
template: '{{bar}}-{{barCopy}}',
});

this.render(`{{foo-bar bar=bar}}`, { bar: 3 });
await this.render(`{{foo-bar bar=bar}}`, { bar: 3 });

this.assertText('3-4');

assert.strictEqual(barCopyDidChangeCount, 1, 'expected observer firing for: barCopy');

runTask(() => set(this.context, 'bar', 7));
set(this.context, 'bar', 7);

await runLoopSettled();

this.assertText('7-8');

Expand Down
Loading

0 comments on commit 4cdbb8c

Please sign in to comment.