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 lts] Fix tag cycles in query parameters #19138

Merged
merged 1 commit into from Sep 30, 2020
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
3 changes: 2 additions & 1 deletion packages/@ember/-internals/metal/lib/alias.ts
Expand Up @@ -12,7 +12,7 @@ import {
validateTag,
valueForTag,
} from '@glimmer/validator';
import { finishLazyChains, getChainTagsForKey } from './chain-tags';
import { CHAIN_PASS_THROUGH, finishLazyChains, getChainTagsForKey } from './chain-tags';
import {
ComputedDescriptor,
Decorator,
Expand Down Expand Up @@ -70,6 +70,7 @@ export class AliasedProperty extends ComputedDescriptor {
setup(obj: object, keyName: string, propertyDesc: PropertyDescriptor, meta: Meta): void {
assert(`Setting alias '${keyName}' on self`, this.altKey !== keyName);
super.setup(obj, keyName, propertyDesc, meta);
CHAIN_PASS_THROUGH.add(this);
}

get(obj: object, keyName: string): any {
Expand Down
13 changes: 5 additions & 8 deletions packages/@ember/-internals/metal/lib/chain-tags.ts
@@ -1,9 +1,8 @@
import { Meta, meta as metaFor, peekMeta } from '@ember/-internals/meta';
import { isObject } from '@ember/-internals/utils';
import { assert, deprecate } from '@ember/debug';
import { DEBUG } from '@glimmer/env';
import { _WeakSet } from '@glimmer/util';
import {
ALLOW_CYCLES,
combine,
createUpdatableTag,
Tag,
Expand All @@ -15,6 +14,8 @@ import {
import { objectAt } from './array';
import { tagForProperty } from './tags';

export const CHAIN_PASS_THROUGH = new _WeakSet();

export function finishLazyChains(meta: Meta, key: string, value: any) {
let lazyTags = meta.readableLazyChainsFor(key);

Expand Down Expand Up @@ -166,7 +167,7 @@ function getChainTags(
// If the key was an alias, we should always get the next value in order to
// bootstrap the alias. This is because aliases, unlike other CPs, should
// always be in sync with the aliased value.
if (descriptor !== undefined && typeof descriptor.altKey === 'string') {
if (CHAIN_PASS_THROUGH.has(descriptor)) {
// tslint:disable-next-line: no-unused-expression
current[segment];
}
Expand All @@ -182,7 +183,7 @@ function getChainTags(
} else {
current = current[segment];
}
} else if (typeof descriptor.altKey === 'string') {
} else if (CHAIN_PASS_THROUGH.has(descriptor)) {
current = current[segment];
} else {
// If the descriptor is defined, then its a normal CP (not an alias, which
Expand Down Expand Up @@ -218,9 +219,5 @@ function getChainTags(
currentMeta = peekMeta(current);
}

if (DEBUG) {
chainTags.forEach((t) => ALLOW_CYCLES!.set(t, true));
}

return chainTags;
}
6 changes: 6 additions & 0 deletions packages/@ember/-internals/metal/lib/computed.ts
Expand Up @@ -2,8 +2,10 @@ import { Meta, meta as metaFor } from '@ember/-internals/meta';
import { inspect, toString } from '@ember/-internals/utils';
import { assert, deprecate, warn } from '@ember/debug';
import EmberError from '@ember/error';
import { DEBUG } from '@glimmer/env';
import { isDestroyed } from '@glimmer/runtime';
import {
ALLOW_CYCLES,
consumeTag,
tagFor,
tagMetaFor,
Expand Down Expand Up @@ -321,6 +323,10 @@ 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
46 changes: 30 additions & 16 deletions packages/@ember/-internals/metal/lib/tracked.ts
@@ -1,8 +1,11 @@
import { meta as metaFor } from '@ember/-internals/meta';
import { isEmberArray } from '@ember/-internals/utils';
import { assert } from '@ember/debug';
import { DEBUG } from '@glimmer/env';
import { consumeTag, dirtyTagFor, tagFor, trackedData } from '@glimmer/validator';
import { CHAIN_PASS_THROUGH } from './chain-tags';
import {
CP_SETTER_FUNCS,
Decorator,
DecoratorPropertyDescriptor,
isElementDescriptor,
Expand Down Expand Up @@ -142,7 +145,7 @@ if (DEBUG) {
setClassicDecorator(tracked);
}

function descriptorForField([_target, key, desc]: [
function descriptorForField([target, key, desc]: [
object,
string,
DecoratorPropertyDescriptor
Expand All @@ -154,25 +157,36 @@ function descriptorForField([_target, key, desc]: [

let { getter, setter } = trackedData<any, any>(key, desc ? desc.initializer : undefined);

return {
function get(this: object): unknown {
let value = getter(this);

// Add the tag of the returned value if it is an array, since arrays
// should always cause updates if they are consumed and then changed
if (Array.isArray(value) || isEmberArray(value)) {
consumeTag(tagFor(value, '[]'));
}

return value;
}

function set(this: object, newValue: unknown): void {
setter(this, newValue);
dirtyTagFor(this, SELF_TAG);
}

let newDesc = {
enumerable: true,
configurable: true,
isTracked: true,

get(): any {
let value = getter(this);
get,
set,
};

// Add the tag of the returned value if it is an array, since arrays
// should always cause updates if they are consumed and then changed
if (Array.isArray(value) || isEmberArray(value)) {
consumeTag(tagFor(value, '[]'));
}
metaFor(target).writeDescriptors(key, newDesc);

return value;
},
CP_SETTER_FUNCS.add(set);
CHAIN_PASS_THROUGH.add(newDesc);

set(newValue: any): void {
setter(this, newValue);
dirtyTagFor(this, SELF_TAG);
},
};
return newDesc;
}
8 changes: 7 additions & 1 deletion packages/@ember/object/compat.ts
Expand Up @@ -2,15 +2,21 @@ import { Meta } from '@ember/-internals/meta';
import {
Decorator,
DecoratorPropertyDescriptor,
descriptorForProperty,
isElementDescriptor,
setClassicDecorator,
} from '@ember/-internals/metal';
import { assert } from '@ember/debug';
import { consumeTag, tagFor, track, UpdatableTag, updateTag } from '@glimmer/validator';

let wrapGetterSetter = function (_target: object, key: string, desc: PropertyDescriptor) {
let wrapGetterSetter = function (target: object, key: string, desc: PropertyDescriptor) {
let { get: originalGet } = desc;

assert(
'You attempted to use @dependentKeyCompat on a property that already has been decorated with either @computed or @tracked. @dependentKeyCompat is only necessary for native getters that are not decorated with @computed.',
descriptorForProperty(target, key) === undefined
);

if (originalGet !== undefined) {
desc.get = function () {
let propertyTag = tagFor(this, key) as UpdatableTag;
Expand Down
40 changes: 40 additions & 0 deletions packages/ember/tests/routing/query_params_test.js
Expand Up @@ -1611,6 +1611,46 @@ moduleFor(
this.assertCurrentPath('/?foo=987');
}

async ['@test Single query params defined with tracked properties can be linked to (and log is present)'](
assert
) {
assert.expect(3);

this.addTemplate(
'application',
`
<LinkTo @route="application" id="the-link">
Home
</LinkTo>
<LinkTo @route="application" @query={{hash foo=(array 123)}} id="the-link-with-params">
'Home (with params)'
</LinkTo>

<!-- this log caused a failure previously, so we leave it to make sure this case is tested -->
{{log this.foo}}
`
);

this.add(
`controller:application`,
class extends Controller {
queryParams = ['foo', 'bar'];
@tracked foo = [];
@tracked bar = [];
}
);

await this.visitAndAssert('/');

document.getElementById('the-link').click();
await runLoopSettled();
this.assertCurrentPath('/');

document.getElementById('the-link-with-params').click();
await runLoopSettled();
this.assertCurrentPath('/?foo=%5B123%5D');
}

async ['@test Single query params defined with native getters and tracked properties can be on the controller and reflected in the url'](
assert
) {
Expand Down