Skip to content

Commit 592b2f8

Browse files
author
Chris Garrett
committed
[BUGFIX lts] Allow computeds to have cycles in their deps
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.
1 parent b0bc40f commit 592b2f8

File tree

3 files changed

+34
-4
lines changed

3 files changed

+34
-4
lines changed

packages/@ember/-internals/metal/lib/alias.ts

+2
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@ import { Meta, meta as metaFor } from '@ember/-internals/meta';
22
import { inspect } from '@ember/-internals/utils';
33
import { assert } from '@ember/debug';
44
import EmberError from '@ember/error';
5+
import { DEBUG } from '@glimmer/env';
56
import {
7+
ALLOW_CYCLES,
68
consumeTag,
79
tagFor,
810
tagMetaFor,

packages/@ember/-internals/metal/lib/computed.ts

+8-4
Original file line numberDiff line numberDiff line change
@@ -323,10 +323,6 @@ export class ComputedProperty extends ComputedDescriptor {
323323
)
324324
);
325325

326-
if (DEBUG) {
327-
ALLOW_CYCLES!.set(tagFor(obj, keyName), true);
328-
}
329-
330326
if (this._hasConfig === false) {
331327
assert(
332328
`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`,
@@ -408,6 +404,10 @@ export class ComputedProperty extends ComputedDescriptor {
408404

409405
if (_dependentKeys !== undefined) {
410406
updateTag(propertyTag!, getChainTagsForKeys(obj, _dependentKeys, tagMeta, meta));
407+
408+
if (DEBUG) {
409+
ALLOW_CYCLES!.set(propertyTag, true);
410+
}
411411
}
412412

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

484484
if (_dependentKeys !== undefined) {
485485
updateTag(propertyTag, getChainTagsForKeys(obj, _dependentKeys, tagMeta, meta));
486+
487+
if (DEBUG) {
488+
ALLOW_CYCLES!.set(propertyTag, true);
489+
}
486490
}
487491

488492
meta.setRevisionFor(keyName, valueForTag(propertyTag));

packages/@ember/-internals/runtime/tests/system/object/computed_test.js

+24
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
getWithDefault,
77
observer,
88
defineProperty,
9+
notifyPropertyChange,
910
} from '@ember/-internals/metal';
1011
import { oneWay as reads } from '@ember/object/computed';
1112
import { A as EmberArray, isArray } from '../../..';
@@ -540,5 +541,28 @@ moduleFor(
540541

541542
assert.ok(true);
542543
}
544+
545+
['@test computeds can have cycles'](assert) {
546+
class CycleObject {
547+
// eslint-disable-next-line getter-return
548+
@computed('bar')
549+
get foo() {}
550+
551+
// eslint-disable-next-line getter-return
552+
@computed('foo')
553+
get bar() {}
554+
}
555+
556+
let obj = new CycleObject();
557+
558+
obj.bar;
559+
obj.foo;
560+
561+
notifyPropertyChange(obj, 'bar');
562+
563+
obj.foo;
564+
565+
assert.ok(true);
566+
}
543567
}
544568
);

0 commit comments

Comments
 (0)