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 beta] Restores the shadowed property set behavior #19232

Merged
merged 1 commit into from
Oct 28, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions packages/@ember/-internals/metal/lib/decorator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,10 @@ export abstract class ComputedDescriptor {
abstract set(obj: object, keyName: string, value: any | null | undefined): any | null | undefined;
}

export let CPGETTERS: WeakSet<() => unknown>;
export let CPSETTERS: WeakSet<(value: unknown) => void>;
export let COMPUTED_GETTERS: WeakSet<() => unknown>;

if (DEBUG) {
CPGETTERS = new WeakSet();
CPSETTERS = new WeakSet();
COMPUTED_GETTERS = new WeakSet();
}

function DESCRIPTOR_GETTER_FUNCTION(name: string, descriptor: ComputedDescriptor): () => unknown {
Expand All @@ -85,7 +83,7 @@ function DESCRIPTOR_GETTER_FUNCTION(name: string, descriptor: ComputedDescriptor
}

if (DEBUG) {
CPGETTERS.add(getter);
COMPUTED_GETTERS.add(getter);
}

return getter;
Expand All @@ -94,18 +92,18 @@ function DESCRIPTOR_GETTER_FUNCTION(name: string, descriptor: ComputedDescriptor
function DESCRIPTOR_SETTER_FUNCTION(
name: string,
descriptor: ComputedDescriptor
): (value: unknown) => void {
function setter(this: object, value: unknown): void {
): (value: any) => void {
let set = function CPSETTER_FUNCTION(this: object, value: any): void {
return descriptor.set(this, name, value);
}
};

if (DEBUG) {
CPSETTERS.add(setter);
}
COMPUTED_SETTERS.add(set);

return setter;
return set;
}

export const COMPUTED_SETTERS = new WeakSet();

export function makeComputedDecorator(
desc: ComputedDescriptor,
DecoratorClass: { prototype: object }
Expand All @@ -119,7 +117,10 @@ export function makeComputedDecorator(
): DecoratorPropertyDescriptor {
assert(
`Only one computed property decorator can be applied to a class field or accessor, but '${key}' was decorated twice. You may have added the decorator to both a getter and setter, which is unnecessary.`,
isClassicDecorator || !propertyDesc || !propertyDesc.get || !CPGETTERS.has(propertyDesc.get)
isClassicDecorator ||
!propertyDesc ||
!propertyDesc.get ||
!COMPUTED_GETTERS.has(propertyDesc.get)
);

let meta = arguments.length === 3 ? metaFor(target) : maybeMeta;
Expand Down
19 changes: 4 additions & 15 deletions packages/@ember/-internals/metal/lib/property_set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
import { assert } from '@ember/debug';
import EmberError from '@ember/error';
import { DEBUG } from '@glimmer/env';
import { CPSETTERS, descriptorForProperty } from './decorator';
import { COMPUTED_SETTERS } from './decorator';
import { isPath } from './path_cache';
import { notifyPropertyChange } from './property_events';
import { _getPath as getPath, getPossibleMandatoryProxyValue } from './property_get';
Expand Down Expand Up @@ -72,21 +72,10 @@ export function set<T = unknown>(obj: object, keyName: string, value: T, toleran
return setPath(obj, keyName, value, tolerant);
}

let descriptor = descriptorForProperty(obj, keyName);
let descriptor = lookupDescriptor(obj, keyName);

if (descriptor !== undefined) {
if (DEBUG) {
let instanceDesc = lookupDescriptor(obj, keyName);

assert(
`Attempted to set \`${toString(
obj
)}.${keyName}\` using Ember.set(), but the property was a computed or tracked property that was shadowed by another property declaration. This can happen if you defined a tracked or computed property on a parent class, and then redefined it on a subclass.`,
instanceDesc && instanceDesc.set && CPSETTERS.has(instanceDesc.set)
);
}

descriptor.set(obj, keyName, value);
if (descriptor !== null && COMPUTED_SETTERS.has(descriptor.set!)) {
obj[keyName] = value;
return value;
}

Expand Down
8 changes: 2 additions & 6 deletions packages/@ember/-internals/metal/lib/tracked.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ import { DEBUG } from '@glimmer/env';
import { consumeTag, dirtyTagFor, tagFor, trackedData } from '@glimmer/validator';
import { CHAIN_PASS_THROUGH } from './chain-tags';
import {
CPGETTERS,
CPSETTERS,
COMPUTED_SETTERS,
Decorator,
DecoratorPropertyDescriptor,
isElementDescriptor,
Expand Down Expand Up @@ -184,10 +183,7 @@ function descriptorForField([target, key, desc]: [
set,
};

if (DEBUG) {
CPGETTERS.add(get);
CPSETTERS.add(set);
}
COMPUTED_SETTERS.add(set);

metaFor(target).writeDescriptors(key, new TrackedDescriptor(get, set));

Expand Down
8 changes: 4 additions & 4 deletions packages/@ember/-internals/metal/tests/tracked/set_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ moduleFor(
}
}

['@test set should throw an error when setting on shadowed properties']() {
['@test set should not throw an error when setting on shadowed properties'](assert) {
class Obj {
@tracked value = 'emberjs';

Expand All @@ -43,9 +43,9 @@ moduleFor(

let newObj = new Obj();

expectAssertion(() => {
set(newObj, 'value', 123);
}, /Attempted to set `\[object Object\].value` using Ember.set\(\), but the property was a computed or tracked property that was shadowed by another property declaration. This can happen if you defined a tracked or computed property on a parent class, and then redefined it on a subclass/);
set(newObj, 'value', 123);

assert.equal(newObj.value, 123, 'it worked');
}
}
);