Skip to content

Commit

Permalink
polish and address feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
Chris Garrett committed Feb 5, 2021
1 parent 4f4b9f0 commit e476b9a
Show file tree
Hide file tree
Showing 10 changed files with 126 additions and 74 deletions.
7 changes: 5 additions & 2 deletions packages/@ember/-internals/glimmer/tests/unit/runtime-resolver-cache-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,11 @@ moduleFor(
this.registerComponent('component-two', { ComponentClass: Two });

// inject layout onto component, share layout with component-one
this.registerComponent('root-component', { ComponentClass: Component });
this.owner.inject('component:root-component', 'layout', 'template:components/component-one');
let Root = Component.extend({
layout: this.owner.lookup('template:components/component-one'),
});

this.registerComponent('root-component', { ComponentClass: Root });

// template instance shared between to template managers
let rootFactory = this.owner.factoryFor('component:root-component');
Expand Down
13 changes: 9 additions & 4 deletions packages/@ember/-internals/routing/lib/system/route.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,17 +100,22 @@ class Route extends EmberObject implements IRoute {
controller!: Controller;
currentModel: unknown;

_bucketCache: BucketCache | undefined;
_bucketCache!: BucketCache;
_internalName!: string;
_names: unknown;
_router: EmberRouter | undefined;
_router!: EmberRouter;

constructor(owner: Owner) {
super(...arguments);

if (owner) {
this._router = owner.lookup('router:main');
this._bucketCache = owner.lookup(P`-bucket-cache:main`);
let router = owner.lookup<EmberRouter>('router:main');
let bucketCache = owner.lookup<BucketCache>(P`-bucket-cache:main`);

assert('Expected route injections to be defined', router && bucketCache);

this._router = router;
this._bucketCache = bucketCache;
this._topLevelViewTemplate = owner.lookup('template:-outlet');
this._environment = owner.lookup('-environment:main');
}
Expand Down
13 changes: 9 additions & 4 deletions packages/@ember/-internals/routing/lib/system/router.ts
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { privatize as P } from '@ember/-internals/container';
import { OutletState as GlimmerOutletState, OutletView } from '@ember/-internals/glimmer';
import { computed, get, notifyPropertyChange, set } from '@ember/-internals/metal';
import { BucketCache } from '@ember/-internals/routing';
import { FactoryClass, getOwner, Owner } from '@ember/-internals/owner';
import { BucketCache } from '@ember/-internals/routing';
import { A as emberA, Evented, Object as EmberObject, typeOf } from '@ember/-internals/runtime';
import Controller from '@ember/controller';
import { assert, deprecate, info } from '@ember/debug';
Expand Down Expand Up @@ -1053,7 +1053,7 @@ class EmberRouter extends EmberObject {
state: TransitionState<Route>,
queryParams: {},
_fromRouterService: boolean
) {
): void {
let routeInfos = state.routeInfos;
let appCache = this._bucketCache;
let qpMeta;
Expand Down Expand Up @@ -1100,6 +1100,9 @@ class EmberRouter extends EmberObject {
}
} else {
let cacheKey = calculateCacheKey(qp.route.fullRouteName, qp.parts, state.params);

assert('expected appCache to be defined', appCache);

queryParams[qp.scopedPropertyName] = appCache.lookup(cacheKey, qp.prop, qp.defaultValue);
}
}
Expand Down Expand Up @@ -1404,7 +1407,7 @@ export function triggerEvent(
ignoreFailure: boolean,
name: string,
args: any[]
) {
): void {
if (!routeInfos) {
if (ignoreFailure) {
return;
Expand All @@ -1427,7 +1430,9 @@ export function triggerEvent(
} else {
// Should only hit here if a non-bubbling error action is triggered on a route.
if (name === 'error') {
handler!._router._markErrorAsHandled(args[0] as Error);
assert('expected handler and handler router to exist', handler && handler._router);

handler._router._markErrorAsHandled(args[0] as Error);
}
return;
}
Expand Down
94 changes: 60 additions & 34 deletions packages/@ember/-internals/runtime/lib/system/core_object.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,12 @@ function initialize(obj, properties) {
// implicit injection takes precedence so need to tell user to rename property on obj
let isInjectedProperty = DEBUG_INJECTION_FUNCTIONS.has(possibleDesc._getter);
if (isInjectedProperty && value !== possibleDesc.get(obj, keyName)) {
implicitInjectionDeprecation(keyName, `You have explicitly defined '${keyName}' for ${inspect(obj)} that does not match the implicit injection for '${keyName}'. Please ensure you are explicitly defining '${keyName}' on ${inspect(obj)}.`);
implicitInjectionDeprecation(
keyName,
`You have explicitly defined a service injection for the '${keyName}' property on ${inspect(
obj
)}. However, a different service or value was injected via implicit injections which overrode your explicit injection. Implicit injections have been deprecated, and will be removed in the near future. In order to prevent breakage, you should inject the same value explicitly that is currently being injected implicitly.`
);
}
}

Expand All @@ -146,30 +151,42 @@ function initialize(obj, properties) {
if (DEBUG) {
// If deprecation, either 1) an accessor descriptor or 2) class field declaration 3) only an implicit injection
let desc = lookupDescriptor(obj, keyName);

if (!injectedProperties.includes(keyName)) {
defineProperty(obj, keyName, null, value, m); // setup mandatory setter
} else if (desc && desc.get) {
if (value !== desc.get.call(obj)) {
implicitInjectionDeprecation(keyName, `You have defined '${keyName}' for ${inspect(obj)} as a getter which does not match the implicit injection for '${keyName}'. Please migrate to '@service ${keyName}'.`);
}
} else if (desc && desc.value) {
if (desc.value !== value) {
} else if (desc) {
// If the property is a value prop, and it isn't the expected value,
// then we can warn the user when they attempt to use the value
if ('value' in desc && desc.value !== value) {
// implicit injection does not match value descriptor set on object
implicitInjectionDeprecation(keyName, `You have defined '${keyName}' for ${inspect(obj)} as a value which does not match the implicit injection for '${keyName}'. Please migrate to '@service ${keyName}'.`);
defineSelfDestructingImplicitInjectionGetter(
obj,
keyName,
value,
`A value was injected implicitly on the '${keyName}' property of an instance of ${inspect(
obj
)}, overwriting the original value which was ${inspect(
desc.value
)}. Implicit injection is now deprecated, please add an explicit injection for this value. If the injected value is a service, consider using the @service decorator.`
);
} else {
// Otherwise, the value is either a getter/setter, or it is the correct value.
// If it is a getter/setter, then we don't know what activating it might do - it could
// be that the user only defined a getter, and so the value will not be set at all. It
// could be that the setter is a no-op that does nothing. Both of these are valid ways
// to "override" an implicit injection, so we can't really warn here. So, assign the
// value like we would normally.
obj[keyName] = value;
}
} else {
// wrapper getter for original adding one time deprecation
// TODO add setter
Object.defineProperty(obj, keyName, {
configurable: true,
get() {
// only want to deprecate on first access so we make this self destructing
Object.defineProperty(obj, keyName, { value });

implicitInjectionDeprecation(keyName, `Implicit injection for property '${keyName}' is now deprecated. Please add an explicit injection for '${keyName}' to ${inspect(obj)}`);
return value;
}
});
defineSelfDestructingImplicitInjectionGetter(
obj,
keyName,
value,
`A value was injected implicitly on the '${keyName}' property of an instance of ${inspect(
obj
)}. Implicit injection is now deprecated, please add an explicit injection for this value. If the injected value is a service, consider using the @service decorator.`
);
}
} else {
obj[keyName] = value;
Expand Down Expand Up @@ -197,6 +214,20 @@ function initialize(obj, properties) {
sendEvent(obj, 'init', undefined, undefined, undefined, m);
}

function defineSelfDestructingImplicitInjectionGetter(obj, keyName, value, message) {
Object.defineProperty(obj, keyName, {
configurable: true,
enumerable: true,
get() {
// only want to deprecate on first access so we make this self destructing
Object.defineProperty(obj, keyName, { value });

implicitInjectionDeprecation(keyName, message);
return value;
},
});
}

/**
`CoreObject` is the base class for all Ember constructs. It establishes a
class system based on Ember's Mixin system, and provides the basis for the
Expand Down Expand Up @@ -1173,21 +1204,16 @@ if (!HAS_NATIVE_SYMBOL) {
});
}

// TODO: customize messages and add more debugging info like obj and mismatch information
function implicitInjectionDeprecation(keyName, msg = null) {
deprecate(
msg,
false,
{
id: 'ember-object.implicit-injection',
until: '4.0.0',
url: 'https://',
for: 'ember-source',
since: {
enabled: '3.24.0',
},
}
);
deprecate(msg, false, {
id: 'implicit-injections',
until: '4.0.0',
url: 'https://deprecations.emberjs.com/v3.x#toc_implicit-injections',
for: 'ember-source',
since: {
enabled: '3.26.0',
},
});
}

export default CoreObject;
62 changes: 38 additions & 24 deletions packages/@ember/-internals/runtime/tests/system/object/create_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ moduleFor(
let obj = owner.lookup('foo:main');
let result;
expectDeprecation(
() => result = obj.foo,
`Implicit injection for property 'foo' is now deprecated. Please add an explicit injection for 'foo' to ${inspect(obj)}`
() => (result = obj.foo),
`A value was injected implicitly on the 'foo' property of an instance of ${inspect(
obj
)}. Implicit injection is now deprecated, please add an explicit injection for this value. If the injected value is a service, consider using the @service decorator.`
);

assert.equal(result.bar, 'foo');
Expand All @@ -64,23 +66,27 @@ moduleFor(
class FooService extends Service {
bar = 'foo';
}
const FooObject = EmberObject.extend();
let FooObject = EmberObject.extend();
owner.register('service:foo', FooService);
owner.register('foo:main', FooObject);
owner.inject('foo:main', 'foo', 'service:foo');

let obj = owner.lookup('foo:main');
let result;
expectDeprecation(
() => result = obj.foo,
`Implicit injection for property 'foo' is now deprecated. Please add an explicit injection for 'foo' to ${inspect(obj)}`
() => (result = obj.foo),
`A value was injected implicitly on the 'foo' property of an instance of ${inspect(
obj
)}. Implicit injection is now deprecated, please add an explicit injection for this value. If the injected value is a service, consider using the @service decorator.`
);

assert.equal(result.bar, 'foo');
assert.equal(obj.foo.bar, 'foo');
}

['@test implicit injections does not raise a deprecation if explicit injection present'](assert) {
['@test implicit injections does not raise a deprecation if explicit injection present'](
assert
) {
expectNoDeprecation();

let owner = buildOwner();
Expand All @@ -99,7 +105,9 @@ moduleFor(
assert.equal(obj.foo.bar, 'foo');
}

['@test raises deprecation if explicit injection is not the same as the implicit injection'](assert) {
['@test raises deprecation if explicit injection is not the same as the implicit injection'](
assert
) {
let owner = buildOwner();

class FooService extends Service {
Expand All @@ -118,13 +126,15 @@ moduleFor(

let result;
expectDeprecation(
() => result = owner.lookup('foo:main'),
`You have explicitly defined 'foo' for FooObject that does not match the implicit injection for 'foo'. Please ensure you are explicitly defining 'foo' on FooObject.`
() => (result = owner.lookup('foo:main')),
/You have explicitly defined a service injection for the 'foo' property on <.*>. However, a different service or value was injected via implicit injections which overrode your explicit injection. Implicit injections have been deprecated, and will be removed in the near future. In order to prevent breakage, you should inject the same value explicitly that is currently being injected implicitly./
);
assert.equal(result.foo.bar, 'bar');
}

['@test does not raise deprecation if descriptor is a value and equal to the implicit deprecation'](assert) {
['@test does not raise deprecation if descriptor is a value and equal to the implicit deprecation'](
assert
) {
expectNoDeprecation();

let owner = buildOwner();
Expand All @@ -147,7 +157,9 @@ moduleFor(
assert.equal(result.foo.bar, 'foo');
}

['@test does raise deprecation if descriptor is a value and not equal to the implicit deprecation'](assert) {
['@test does raise deprecation if descriptor is a value and not equal to the implicit deprecation'](
assert
) {
let owner = buildOwner();

class FooService extends Service {
Expand All @@ -164,15 +176,15 @@ moduleFor(
owner.register('foo:main', FooObject);
owner.inject('foo:main', 'foo', 'service:bar');

let result;
expectDeprecation(
() => result = owner.lookup('foo:main'),
`You have defined 'foo' for FooObject as a value which does not match the implicit injection for 'foo'. Please migrate to '@service foo'.`
);
assert.equal(result.foo.bar, 'foo');
expectDeprecation(() => {
let result = owner.lookup('foo:main');
assert.equal(result.foo.bar, 'bar');
}, /A value was injected implicitly on the 'foo' property of an instance of <.*>, overwriting the original value which was <.*>. Implicit injection is now deprecated, please add an explicit injection for this value/);
}

['@test does not raise deprecation if descriptor is a getter and equal to the implicit deprecation'](assert) {
['@test does not raise deprecation if descriptor is a getter and equal to the implicit deprecation'](
assert
) {
expectNoDeprecation();

let owner = buildOwner();
Expand All @@ -187,6 +199,8 @@ moduleFor(
get foo() {
return getOwner(this).lookup('service:foo');
}

set foo(_) {}
}
owner.register('service:foo', FooService);
owner.register('service:bar', BarService);
Expand All @@ -197,7 +211,9 @@ moduleFor(
assert.equal(result.foo.bar, 'foo');
}

['@test does raise deprecation if descriptor is a getter and not equal to the implicit deprecation'](assert) {
['@test does not raise deprecation if descriptor is a getter and not equal to the implicit deprecation'](
assert
) {
let owner = buildOwner();

class FooService extends Service {
Expand All @@ -210,17 +226,15 @@ moduleFor(
get foo() {
return getOwner(this).lookup('service:foo');
}

set foo(_) {}
}
owner.register('service:foo', FooService);
owner.register('service:bar', BarService);
owner.register('foo:main', FooObject);
owner.inject('foo:main', 'foo', 'service:bar');

let result;
expectDeprecation(
() => result = owner.lookup('foo:main'),
`You have defined 'foo' for FooObject as a getter which does not match the implicit injection for 'foo'. Please migrate to '@service foo'.`
);
let result = owner.lookup('foo:main');
assert.equal(result.foo.bar, 'foo');
}

Expand Down
3 changes: 0 additions & 3 deletions packages/@ember/-internals/utils/lib/inspect.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,6 @@ function inspectValue(value: any | null | undefined, depth: number, seen?: WeakS
if (value.toString === objectToString || value.toString === undefined) {
break;
}
if (value.constructor && typeof value.constructor === 'function' && value.constructor.name !== 'Class') {
return value.constructor.name;
}
// custom toString
return value.toString();
case 'function':
Expand Down
1 change: 0 additions & 1 deletion packages/@ember/application/tests/application_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { _loaded } from '@ember/application';
import Controller from '@ember/controller';
import { Object as EmberObject } from '@ember/-internals/runtime';
import { setTemplates } from '@ember/-internals/glimmer';
import { privatize as P } from '@ember/-internals/container';
import { assign } from '@ember/polyfills';
import {
moduleFor,
Expand Down
4 changes: 4 additions & 0 deletions packages/@ember/application/tests/dependency_injection_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ moduleFor(
}

['@test injections'](assert) {
expectDeprecation(
/A value was injected implicitly on the 'fruit' property of an instance of <.*>. Implicit injection is now deprecated, please add an explicit injection for this value/
);

application.inject('model', 'fruit', 'fruit:favorite');
application.inject('model:user', 'communication', 'communication:main');

Expand Down
Loading

0 comments on commit e476b9a

Please sign in to comment.