Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Chris Garrett committed Aug 9, 2020
1 parent ef0e277 commit 1ad23fe
Show file tree
Hide file tree
Showing 8 changed files with 59 additions and 48 deletions.
23 changes: 11 additions & 12 deletions packages/@ember/-internals/container/lib/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ if (DEBUG) {
* Wrap a factory manager in a proxy which will not permit properties to be
* set on the manager.
*/
function wrapManagerInDeprecationProxy<T, C>(manager: FactoryManager<T, C>) {
function wrapManagerInDeprecationProxy<T, C>(manager: FactoryManager<T, C>): FactoryManager<T, C> {
if (HAS_NATIVE_PROXY) {
let validator = {
set(_obj: T, prop: keyof T) {
Expand All @@ -267,8 +267,7 @@ function wrapManagerInDeprecationProxy<T, C>(manager: FactoryManager<T, C>) {
},
};

let proxy = new Proxy(proxiedManager, validator as any);
// FACTORY_FOR.set(proxy, manager);
return new Proxy(proxiedManager, validator as any) as any;
}

return manager;
Expand Down Expand Up @@ -434,7 +433,7 @@ function instantiateFactory(
}

interface BuildInjectionsResult {
injections: { [key: string]: object } | undefined;
injections: { [key: string]: unknown };
isDynamic: boolean;
}

Expand All @@ -448,9 +447,6 @@ function processInjections(
}

let hash = result.injections;
if (hash === undefined) {
hash = result.injections = {};
}

for (let i = 0; i < injections.length; i++) {
let { property, specifier, source } = injections[i];
Expand All @@ -472,8 +468,12 @@ function buildInjections(
typeInjections: Injection[],
injections: Injection[]
): BuildInjectionsResult {
let injectionsHash = {};

setOwner(injectionsHash, container.owner!);

let result: BuildInjectionsResult = {
injections: undefined,
injections: injectionsHash,
isDynamic: false,
};

Expand Down Expand Up @@ -560,7 +560,7 @@ class FactoryManager<T, C> {
readonly fullName: string;
readonly normalizedName: string;
private madeToString: string | undefined;
injections: { [key: string]: object } | undefined;
injections: { [key: string]: unknown } | undefined;

constructor(
container: Container,
Expand Down Expand Up @@ -598,9 +598,8 @@ class FactoryManager<T, C> {
let props = this.injections;
if (props === undefined) {
let { injections, isDynamic } = injectionsFor(this.container, this.normalizedName);
props = injections || {};
setOwner(props, this.owner!);
props[INIT_FACTORY as string] = this;
setFactoryFor(injections, this);
props = injections;

if (!isDynamic) {
this.injections = injections;
Expand Down
2 changes: 2 additions & 0 deletions packages/@ember/-internals/container/tests/owner_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ moduleFor(

let legacyOwner;

// This is not something we expect to happen a lot, but does exist currently
// in the wild: https://github.com/hjdivad/ember-m3/pull/822
for (let key in obj) {
legacyOwner = key;
}
Expand Down
28 changes: 7 additions & 21 deletions packages/@ember/-internals/glimmer/lib/renderer.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ENV } from '@ember/-internals/environment';
import { OWNER, Owner } from '@ember/-internals/owner';
import { getOwner, Owner } from '@ember/-internals/owner';
import { getViewElement, getViewId, OwnedTemplateMeta } from '@ember/-internals/views';
import { assert } from '@ember/debug';
import { backburner, getCurrentRunLoop } from '@ember/runloop';
Expand Down Expand Up @@ -543,22 +543,15 @@ export abstract class Renderer {
}

export class InertRenderer extends Renderer {
static create({
[OWNER]: owner,
document,
env,
rootTemplate,
_viewRegistry,
builder,
}: {
[OWNER]: Owner;
static create(props: {
document: SimpleDocument;
env: { isInteractive: boolean; hasDOM: boolean };
rootTemplate: TemplateFactory;
_viewRegistry: any;
builder: any;
}) {
return new this(owner, document, env, rootTemplate, _viewRegistry, false, builder);
let { document, env, rootTemplate, _viewRegistry, builder } = props;
return new this(getOwner(props), document, env, rootTemplate, _viewRegistry, false, builder);
}

getElement(_view: unknown): Option<SimpleElement> {
Expand All @@ -569,22 +562,15 @@ export class InertRenderer extends Renderer {
}

export class InteractiveRenderer extends Renderer {
static create({
[OWNER]: owner,
document,
env,
rootTemplate,
_viewRegistry,
builder,
}: {
[OWNER]: Owner;
static create(props: {
document: SimpleDocument;
env: { isInteractive: boolean; hasDOM: boolean };
rootTemplate: TemplateFactory;
_viewRegistry: any;
builder: any;
}) {
return new this(owner, document, env, rootTemplate, _viewRegistry, true, builder);
let { document, env, rootTemplate, _viewRegistry, builder } = props;
return new this(getOwner(props), document, env, rootTemplate, _viewRegistry, true, builder);
}

getElement(view: unknown): Option<SimpleElement> {
Expand Down
4 changes: 2 additions & 2 deletions packages/@ember/-internals/glimmer/lib/views/outlet.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { OWNER, Owner } from '@ember/-internals/owner';
import { getOwner, Owner } from '@ember/-internals/owner';
import { assign } from '@ember/polyfills';
import { schedule } from '@ember/runloop';
import { SimpleElement } from '@simple-dom/interface';
Expand Down Expand Up @@ -35,7 +35,7 @@ export default class OutletView {

static create(options: any) {
let { _environment, renderer, template: templateFactory } = options;
let owner = options[OWNER];
let owner = getOwner(options);
let template = templateFactory(owner);
return new OutletView(_environment, renderer, owner, template);
}
Expand Down
8 changes: 5 additions & 3 deletions packages/@ember/-internals/metal/lib/events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,13 @@ export function sendEvent(
if (!target) {
target = obj;
}
if (typeof method === 'string' || typeof method === 'symbol') {
method = target[method] as Function;

let type = typeof method;
if (type === 'string' || type === 'symbol') {
method = target[method as string] as Function;
}

method.apply(target, params);
(method as Function).apply(target, params);
}
return true;
}
Expand Down
4 changes: 3 additions & 1 deletion packages/@ember/-internals/runtime/lib/system/core_object.js
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,9 @@ function initialize(obj, properties) {
*/
class CoreObject {
constructor(owner) {
// pluck off factory and set it as the instance factory
// setOwner has to set both OWNER and LEGACY_OWNER for backwards compatibility, and
// LEGACY_OWNER is enumerable, so setting it would add an enumerable property to the object,
// so we just set `OWNER` directly here.
this[OWNER] = owner;

// prepare prototype...
Expand Down
8 changes: 1 addition & 7 deletions packages/@ember/-internals/runtime/lib/system/object.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/

import { getFactoryFor, INIT_FACTORY } from '@ember/-internals/container';
import { OWNER, setOwner } from '@ember/-internals/owner';
import { OWNER } from '@ember/-internals/owner';
import { HAS_NATIVE_SYMBOL, symbol, setName } from '@ember/-internals/utils';
import { addListener } from '@ember/-internals/metal';
import CoreObject from './core_object';
Expand Down Expand Up @@ -65,12 +65,6 @@ FrameworkObject = class FrameworkObject extends CoreObject {
let factory = getFactoryFor(this);
return factory !== undefined && factory.fullName;
}

constructor(owner) {
super();

setOwner(this, owner);
}
};

Observable.apply(FrameworkObject.prototype);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { getOwner, setOwner } from '@ember/-internals/owner';
import { get, set } from '@ember/-internals/metal';
import { get, set, observer } from '@ember/-internals/metal';
import CoreObject from '../../lib/system/core_object';
import { moduleFor, AbstractTestCase, buildOwner } from 'internal-test-helpers';
import { moduleFor, AbstractTestCase, buildOwner, runLoopSettled } from 'internal-test-helpers';
import { track } from '@glimmer/validator';

moduleFor(
Expand Down Expand Up @@ -85,6 +85,32 @@ moduleFor(
}).create(options);
}

async ['@test observed properties are enumerable when set GH#14594'](assert) {
let callCount = 0;
let Test = CoreObject.extend({
myProp: null,
anotherProp: undefined,
didChangeMyProp: observer('myProp', function() {
callCount++;
}),
});

let test = Test.create();
set(test, 'id', '3');
set(test, 'myProp', { id: 1 });

assert.deepEqual(Object.keys(test).sort(), ['id', 'myProp']);

set(test, 'anotherProp', 'nice');

assert.deepEqual(Object.keys(test).sort(), ['anotherProp', 'id', 'myProp']);
await runLoopSettled();

assert.equal(callCount, 1);

test.destroy();
}

['@test native getters/setters do not cause rendering invalidation during init'](assert) {
let objectMeta = Object.create(null);

Expand Down

0 comments on commit 1ad23fe

Please sign in to comment.