Skip to content

Commit

Permalink
[BUGFIX lts] Allow computeds to have cycles in their deps
Browse files Browse the repository at this point in the history
The bugfix for disallowed cycles in tracked props in #19138 attempted
to also narrow the number of cycles that are allowed in general. Cycles
should only be allowed for computed property deps, for legacy support
reasons. The logic to allow cycles for these tags in particular was
mistakenly added to `setup`, which runs on the _prototype_ of the class.
This meant that _instance_ computed props were not allowed to have
cycles, and this was causing failures in the ecosystem.

Added a test that failed and is fixed with this change. I also attempted
to create a cycle with `@alias` since it uses a different
implementation, but I wasn't able to create one which didn't result in
a Maximum Callstack style recursion error, so I think it's just not
possible at all anyways, since `@alias` is eager always and never
caches.
  • Loading branch information
Chris Garrett committed Oct 1, 2020
1 parent b0bc40f commit 592b2f8
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 4 deletions.
2 changes: 2 additions & 0 deletions packages/@ember/-internals/metal/lib/alias.ts
Expand Up @@ -2,7 +2,9 @@ import { Meta, meta as metaFor } from '@ember/-internals/meta';
import { inspect } from '@ember/-internals/utils';
import { assert } from '@ember/debug';
import EmberError from '@ember/error';
import { DEBUG } from '@glimmer/env';
import {
ALLOW_CYCLES,
consumeTag,
tagFor,
tagMetaFor,
Expand Down
12 changes: 8 additions & 4 deletions packages/@ember/-internals/metal/lib/computed.ts
Expand Up @@ -323,10 +323,6 @@ export class ComputedProperty extends ComputedDescriptor {
)
);

if (DEBUG) {
ALLOW_CYCLES!.set(tagFor(obj, keyName), true);
}

if (this._hasConfig === false) {
assert(
`Attempted to use @computed on ${keyName}, but it did not have a getter or a setter. You must either pass a get a function or getter/setter to @computed directly (e.g. \`@computed({ get() { ... } })\`) or apply @computed directly to a getter/setter`,
Expand Down Expand Up @@ -408,6 +404,10 @@ export class ComputedProperty extends ComputedDescriptor {

if (_dependentKeys !== undefined) {
updateTag(propertyTag!, getChainTagsForKeys(obj, _dependentKeys, tagMeta, meta));

if (DEBUG) {
ALLOW_CYCLES!.set(propertyTag, true);
}
}

meta.setValueFor(keyName, ret);
Expand Down Expand Up @@ -483,6 +483,10 @@ export class ComputedProperty extends ComputedDescriptor {

if (_dependentKeys !== undefined) {
updateTag(propertyTag, getChainTagsForKeys(obj, _dependentKeys, tagMeta, meta));

if (DEBUG) {
ALLOW_CYCLES!.set(propertyTag, true);
}
}

meta.setRevisionFor(keyName, valueForTag(propertyTag));
Expand Down
Expand Up @@ -6,6 +6,7 @@ import {
getWithDefault,
observer,
defineProperty,
notifyPropertyChange,
} from '@ember/-internals/metal';
import { oneWay as reads } from '@ember/object/computed';
import { A as EmberArray, isArray } from '../../..';
Expand Down Expand Up @@ -540,5 +541,28 @@ moduleFor(

assert.ok(true);
}

['@test computeds can have cycles'](assert) {
class CycleObject {
// eslint-disable-next-line getter-return
@computed('bar')
get foo() {}

// eslint-disable-next-line getter-return
@computed('foo')
get bar() {}
}

let obj = new CycleObject();

obj.bar;
obj.foo;

notifyPropertyChange(obj, 'bar');

obj.foo;

assert.ok(true);
}
}
);

0 comments on commit 592b2f8

Please sign in to comment.