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] Prevents the recursive redefinition of root chains #16857

Merged
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/ember-meta/lib/meta.ts
Expand Up @@ -323,12 +323,12 @@ export class Meta {
);
let ret = this._chains;
if (ret === undefined) {
if (this.parent === null) {
ret = create(this.source);
} else {
ret = this.parent.writableChains(create).copy(this.source);
this._chains = ret = create(this.source);

if (this.parent !== null) {
let parentChains = this.parent.writableChains(create);
parentChains.copyTo(ret);
}
this._chains = ret;
}
return ret;
}
Expand Down
6 changes: 2 additions & 4 deletions packages/ember-metal/lib/chains.ts
Expand Up @@ -208,18 +208,16 @@ class ChainNode {
}

// copies a top level object only
copy(obj: any) {
let ret = makeChainNode(obj);
copyTo(target: ChainNode) {
let paths = this.paths;
if (paths !== undefined) {
let path;
for (path in paths) {
if (paths[path] > 0) {
ret.add(path);
target.add(path);
}
}
}
return ret;
}

// called on the root node of a chain to setup watchers on the specified
Expand Down
51 changes: 51 additions & 0 deletions packages/ember-metal/tests/chains_test.js
Expand Up @@ -5,6 +5,7 @@ import {
finishChains,
defineProperty,
computed,
alias,
notifyPropertyChange,
watch,
unwatch,
Expand Down Expand Up @@ -152,5 +153,55 @@ moduleFor(
assert.equal(watcherCount(obj.a, 'b.c'), 0);
assert.equal(watcherCount(obj.a.b, 'c'), 0);
}

['@test writable chains is not defined more than once'](assert) {
assert.expect(0);
function didChange() {}

let obj = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer using class syntax, and defining the CPs on prototype then calling finish chains in the constructor

Copy link
Member

Choose a reason for hiding this comment

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

Mind sending a follow up PR for that?

foo: {
bar: {
baz: {
value: 123,
},
},
},
};

// Setup object like a constructor, which delays initializing values in chains
let parentMeta = meta(obj);
parentMeta.proto = obj;

// Define a standard computed property, which will eventually setup dependencies
defineProperty(
obj,
'bar',
computed('foo.bar', {
get() {
return this.foo.bar;
},
})
);

// Define some aliases, which will proxy chains along
defineProperty(obj, 'baz', alias('bar.baz'));
defineProperty(obj, 'value', alias('baz.value'));

// Define an observer, which will eagerly attempt to setup chains and watch
// their values. This follows the aliases eagerly, and forces the first
// computed to actually set up its values/dependencies for chains. If
// writableChains was not already defined, this results in multiple root
// chain nodes being defined on the same object meta.
addObserver(obj, 'value', null, didChange);

let childObj = Object.create(obj);

let childMeta = meta(childObj);

finishChains(childMeta);

// If we can unwatch the root computed, then chains was not overwritten
unwatch(childObj, 'foo.bar');
}
}
);