From 3f89eb986766b4b9a1aff727c0a88920cf3665cf Mon Sep 17 00:00:00 2001 From: Matthew Beale Date: Tue, 13 Feb 2018 08:14:33 -0800 Subject: [PATCH] Introduce source to injections Ember templates use a `source` argument to the container. This hints where to look for "local lookup", and also what namespace to look in for a partial specifier. For example a template in an addon's `src` tree can invoke components and helpers in that tree becuase it has a `source` argument passed to lookups. This introduces the same functionality for service injections. A service can now accept a `source` argumnet and its lookup will apply to the namespace of that file. As with templates, this is intended to be a build-time feature. We can run an AST transform on JS to add this `source` argumnet to any injections. --- packages/container/lib/container.js | 18 +-- packages/container/lib/registry.js | 32 +++--- packages/container/tests/container_test.js | 13 ++- .../tests/system/application_instance_test.js | 23 ++++ .../tests/test-helpers/registry-check.js | 2 +- .../components/local-lookup-test.js | 32 ++++++ packages/ember-metal/lib/injected_property.js | 10 +- packages/ember-runtime/lib/inject.js | 8 +- .../ember-runtime/lib/system/core_object.js | 5 +- packages/ember-runtime/tests/inject_test.js | 7 +- .../ember/tests/service_injection_test.js | 105 +++++++++++++++++- 11 files changed, 220 insertions(+), 35 deletions(-) diff --git a/packages/container/lib/container.js b/packages/container/lib/container.js index 68b0026be9f..eba0b70122c 100644 --- a/packages/container/lib/container.js +++ b/packages/container/lib/container.js @@ -313,12 +313,15 @@ function buildInjections(container, injections) { container.registry.validateInjections(injections); } - let injection; for (let i = 0; i < injections.length; i++) { - injection = injections[i]; - hash[injection.property] = lookup(container, injection.fullName); + let {property, specifier, source} = injections[i]; + if (source) { + hash[property] = lookup(container, specifier, {source}); + } else { + hash[property] = lookup(container, specifier); + } if (!isDynamic) { - isDynamic = !isSingleton(container, injection.fullName); + isDynamic = !isSingleton(container, specifier); } } } @@ -355,12 +358,13 @@ function resetCache(container) { } function resetMember(container, fullName) { - let member = container.cache[fullName]; + let cacheKey = container._resolverCacheKey(fullName); + let member = container.cache[cacheKey]; - delete container.factoryManagerCache[fullName]; + delete container.factoryManagerCache[cacheKey]; if (member) { - delete container.cache[fullName]; + delete container.cache[cacheKey]; if (member.destroy) { member.destroy(); diff --git a/packages/container/lib/registry.js b/packages/container/lib/registry.js index 820d0554710..bcd2e9ff5d1 100644 --- a/packages/container/lib/registry.js +++ b/packages/container/lib/registry.js @@ -151,9 +151,10 @@ export default class Registry { assert(`Attempting to register an unknown factory: '${fullName}'`, factory !== undefined); let normalizedName = this.normalize(fullName); - assert(`Cannot re-register: '${fullName}', as it has already been resolved.`, !this._resolveCache[normalizedName]); + let cacheKey = this.resolverCacheKey(normalizedName); + assert(`Cannot re-register: '${fullName}', as it has already been resolved.`, !this._resolveCache[cacheKey]); - this._failSet.delete(normalizedName); + this._failSet.delete(cacheKey); this.registrations[normalizedName] = factory; this._options[normalizedName] = options; } @@ -179,13 +180,14 @@ export default class Registry { assert('fullName must be a proper full name', this.isValidFullName(fullName)); let normalizedName = this.normalize(fullName); + let cacheKey = this.resolverCacheKey(normalizedName); this._localLookupCache = Object.create(null); delete this.registrations[normalizedName]; - delete this._resolveCache[normalizedName]; + delete this._resolveCache[cacheKey]; delete this._options[normalizedName]; - this._failSet.delete(normalizedName); + this._failSet.delete(cacheKey); } /** @@ -450,7 +452,7 @@ export default class Registry { let injections = this._typeInjections[type] || (this._typeInjections[type] = []); - injections.push({ property, fullName }); + injections.push({ property, specifier: fullName }); } /** @@ -514,7 +516,7 @@ export default class Registry { let injections = this._injections[normalizedName] || (this._injections[normalizedName] = []); - injections.push({ property, fullName: normalizedInjectionName }); + injections.push({ property, specifier: normalizedInjectionName }); } /** @@ -567,11 +569,7 @@ export default class Registry { } resolverCacheKey(name, options) { - if (!EMBER_MODULE_UNIFICATION) { - return name; - } - - return (options && options.source) ? `${options.source}:${name}` : name; + return `${name}\0${(options && options.source) || ''}`; } /** @@ -625,11 +623,13 @@ if (DEBUG) { for (let key in hash) { if (hash.hasOwnProperty(key)) { - assert(`Expected a proper full name, given '${hash[key]}'`, this.isValidFullName(hash[key])); + let { specifier, source } = hash[key]; + assert(`Expected a proper full name, given '${specifier}'`, this.isValidFullName(specifier)); injections.push({ property: key, - fullName: hash[key] + specifier, + source }); } } @@ -640,12 +640,10 @@ if (DEBUG) { Registry.prototype.validateInjections = function(injections) { if (!injections) { return; } - let fullName; - for (let i = 0; i < injections.length; i++) { - fullName = injections[i].fullName; + let {specifier, source} = injections[i]; - assert(`Attempting to inject an unknown injection: '${fullName}'`, this.has(fullName)); + assert(`Attempting to inject an unknown injection: '${specifier}'`, this.has(specifier, {source})); } }; } diff --git a/packages/container/tests/container_test.js b/packages/container/tests/container_test.js index a87a244dc66..f4f731a2679 100644 --- a/packages/container/tests/container_test.js +++ b/packages/container/tests/container_test.js @@ -388,7 +388,7 @@ moduleFor('Container', class extends AbstractTestCase { assert.deepEqual(resolveWasCalled, ['foo:post']); } - ['@test A factory\'s lazy injections are validated when first instantiated'](assert) { + [`@test A factory's lazy injections are validated when first instantiated`](assert) { let registry = new Registry(); let container = registry.container(); let Apple = factory(); @@ -396,7 +396,10 @@ moduleFor('Container', class extends AbstractTestCase { Apple.reopenClass({ _lazyInjections() { - return ['orange:main', 'banana:main']; + return [ + {specifier: 'orange:main'}, + {specifier: 'banana:main'} + ]; } }); @@ -419,7 +422,9 @@ moduleFor('Container', class extends AbstractTestCase { Apple.reopenClass({ _lazyInjections: () => { assert.ok(true, 'should call lazy injection method'); - return ['orange:main']; + return [ + {specifier: 'orange:main'} + ]; } }); @@ -676,7 +681,7 @@ if (EMBER_MODULE_UNIFICATION) { let result = container.lookup(lookup, { source: expectedSource }); this.assert.ok(result instanceof PrivateComponent, 'The correct factory was provided'); - this.assert.ok(container.cache[`template:routes/application:component:my-input`] instanceof PrivateComponent, + this.assert.ok(container.cache[`component:my-input\0template:routes/application`] instanceof PrivateComponent, 'The correct factory was stored in the cache with the correct key which includes the source.'); } }); diff --git a/packages/ember-application/tests/system/application_instance_test.js b/packages/ember-application/tests/system/application_instance_test.js index 7b1d8c610f0..e70f406e82d 100644 --- a/packages/ember-application/tests/system/application_instance_test.js +++ b/packages/ember-application/tests/system/application_instance_test.js @@ -120,6 +120,29 @@ moduleFor('ApplicationInstance', class extends TestCase { assert.notStrictEqual(postController1, postController2, 'lookup creates a brand new instance, because the previous one was reset'); } + ['@skip unregistering a factory clears caches with source of that factory'](assert) { + assert.expect(1); + + appInstance = run(() => ApplicationInstance.create({ application })); + + let PostController1 = factory(); + let PostController2 = factory(); + + appInstance.register('controller:post', PostController1); + + let postController1 = appInstance.lookup('controller:post'); + let postControllerLookupWithSource = appInstance.lookup('controller:post', {source: 'doesnt-even-matter'}); + + appInstance.unregister('controller:post'); + appInstance.register('controller:post', PostController2); + + // The cache that is source-specific is not cleared + assert.ok( + postControllerLookupWithSource !== appInstance.lookup('controller:post', {source: 'doesnt-even-matter'}), + 'lookup with source creates a new instance' + ); + } + ['can build and boot a registered engine'](assert) { assert.expect(11); diff --git a/packages/ember-application/tests/test-helpers/registry-check.js b/packages/ember-application/tests/test-helpers/registry-check.js index 868b43a544b..dd75bf7c65a 100644 --- a/packages/ember-application/tests/test-helpers/registry-check.js +++ b/packages/ember-application/tests/test-helpers/registry-check.js @@ -18,7 +18,7 @@ export function verifyInjection(assert, owner, fullName, property, injectionName for (let i = 0, l = injections.length; i < l; i++) { injection = injections[i]; - if (injection.property === property && injection.fullName === normalizedName) { + if (injection.property === property && injection.specifier === normalizedName) { hasInjection = true; break; } diff --git a/packages/ember-glimmer/tests/integration/components/local-lookup-test.js b/packages/ember-glimmer/tests/integration/components/local-lookup-test.js index e04e88f05fc..3f21e49d5a8 100644 --- a/packages/ember-glimmer/tests/integration/components/local-lookup-test.js +++ b/packages/ember-glimmer/tests/integration/components/local-lookup-test.js @@ -1,5 +1,7 @@ import { moduleFor, RenderingTest } from '../../utils/test-case'; +import { compile } from 'ember-template-compiler'; import { ModuleBasedTestResolver } from 'internal-test-helpers'; +import { moduleFor as applicationModuleFor, ApplicationTestCase } from 'internal-test-helpers'; import { Component } from '../../utils/helpers'; import { EMBER_MODULE_UNIFICATION } from 'ember/features'; import { helper, Helper } from 'ember-glimmer'; @@ -302,3 +304,33 @@ if (EMBER_MODULE_UNIFICATION) { } }); } + +if (EMBER_MODULE_UNIFICATION) { + applicationModuleFor('Components test: local lookup with resolution referrer (MU)', class extends ApplicationTestCase { + ['@test Ensure that the same specifier with two sources does not share a cache key'](assert) { + this.add({ + specifier: 'template:components/x-not-shared', + source: 'template:x-top' + }, compile('child-x-not-shared')); + + this.add({ + specifier: 'template:components/x-top', + source: 'template:application' + }, compile('top-level-x-top ({{x-not-shared}})', { moduleName: 'x-top' })); + + this.add({ + specifier: 'template:components/x-not-shared', + source: 'template:application' + }, compile('top-level-x-not-shared')); + + this.addTemplate('application', '{{x-not-shared}} {{x-top}} {{x-not-shared}} {{x-top}}'); + + return this.visit('/').then(() => { + assert.equal( + this.element.textContent, + 'top-level-x-not-shared top-level-x-top (child-x-not-shared) top-level-x-not-shared top-level-x-top (child-x-not-shared)' + ); + }); + } + }); +} diff --git a/packages/ember-metal/lib/injected_property.js b/packages/ember-metal/lib/injected_property.js index 698c1267434..6eaeb6f4d18 100644 --- a/packages/ember-metal/lib/injected_property.js +++ b/packages/ember-metal/lib/injected_property.js @@ -21,9 +21,10 @@ import { descriptorFor } from './meta'; to the property's name @private */ -export default function InjectedProperty(type, name) { +export default function InjectedProperty(type, name, options) { this.type = type; this.name = name; + this.source = options ? options.source : undefined; this._super$Constructor(injectedPropertyGet); AliasedPropertyPrototype.oneWay.call(this); @@ -36,7 +37,12 @@ function injectedPropertyGet(keyName) { assert(`InjectedProperties should be defined with the inject computed property macros.`, desc && desc.type); assert(`Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.`, owner); - return owner.lookup(`${desc.type}:${desc.name || keyName}`); + let specifier = `${desc.type}:${desc.name || keyName}`; + if (desc.source) { + return owner.lookup(specifier, {source: desc.source}); + } else { + return owner.lookup(specifier); + } } InjectedProperty.prototype = Object.create(Descriptor.prototype); diff --git a/packages/ember-runtime/lib/inject.js b/packages/ember-runtime/lib/inject.js index 1360e5f6263..18992ba30f0 100644 --- a/packages/ember-runtime/lib/inject.js +++ b/packages/ember-runtime/lib/inject.js @@ -1,5 +1,7 @@ import { InjectedProperty, descriptorFor } from 'ember-metal'; import { assert } from 'ember-debug'; +import { EMBER_MODULE_UNIFICATION } from 'ember/features'; + /** @module ember */ @@ -35,7 +37,11 @@ const typeValidators = {}; export function createInjectionHelper(type, validator) { typeValidators[type] = validator; - inject[type] = name => new InjectedProperty(type, name); + if (EMBER_MODULE_UNIFICATION) { + inject[type] = (name, options) => new InjectedProperty(type, name, options); + } else { + inject[type] = name => new InjectedProperty(type, name); + } } /** diff --git a/packages/ember-runtime/lib/system/core_object.js b/packages/ember-runtime/lib/system/core_object.js index 489b1c0fcf9..6133844edfe 100644 --- a/packages/ember-runtime/lib/system/core_object.js +++ b/packages/ember-runtime/lib/system/core_object.js @@ -1046,7 +1046,10 @@ if (DEBUG) { for (key in proto) { desc = descriptorFor(proto, key); if (desc instanceof InjectedProperty) { - injections[key] = `${desc.type}:${desc.name || key}`; + injections[key] = { + specifier: `${desc.type}:${desc.name || key}`, + source: desc.source + }; } } diff --git a/packages/ember-runtime/tests/inject_test.js b/packages/ember-runtime/tests/inject_test.js index fe3f68397bd..54988df6b1a 100644 --- a/packages/ember-runtime/tests/inject_test.js +++ b/packages/ember-runtime/tests/inject_test.js @@ -61,6 +61,11 @@ if (DEBUG) { bar: new InjectedProperty('quux') }); - assert.deepEqual(AnObject._lazyInjections(), { 'foo': 'foo:bar', 'bar': 'quux:bar' }, 'should return injected container keys'); + assert.deepEqual( + AnObject._lazyInjections(), + { + 'foo': { specifier: 'foo:bar', source: undefined }, + 'bar': { specifier: 'quux:bar', source: undefined } + }, 'should return injected container keys'); }); } diff --git a/packages/ember/tests/service_injection_test.js b/packages/ember/tests/service_injection_test.js index 675ef04bef2..9901a708953 100644 --- a/packages/ember/tests/service_injection_test.js +++ b/packages/ember/tests/service_injection_test.js @@ -2,7 +2,7 @@ import { Controller } from 'ember-runtime'; import { moduleFor, ApplicationTestCase } from 'internal-test-helpers'; import { inject, Service } from 'ember-runtime'; import { computed } from 'ember-metal'; -import { EMBER_METAL_ES5_GETTERS } from 'ember/features'; +import { EMBER_METAL_ES5_GETTERS, EMBER_MODULE_UNIFICATION } from 'ember/features'; moduleFor('Service Injection', class extends ApplicationTestCase { @@ -43,3 +43,106 @@ if (EMBER_METAL_ES5_GETTERS) { } }); } + +if (EMBER_MODULE_UNIFICATION) { + moduleFor('Service Injection (MU)', class extends ApplicationTestCase { + ['@test Service can be injected with source and is resolved'](assert) { + let source = 'src/ui/routes/application/controller'; + this.add('controller:application', Controller.extend({ + myService: inject.service('my-service', { source }) + })); + let MyService = Service.extend(); + this.add({ + specifier: 'service:my-service', + source + }, MyService); + + return this.visit('/').then(() => { + let controller = this.applicationInstance.lookup('controller:application'); + + assert.ok(controller.get('myService') instanceof MyService); + }); + } + + ['@test Services can be injected with same name, different source, but same resolution result, and share an instance'](assert) { + let routeASource = 'src/ui/routes/route-a/controller'; + let routeBSource = 'src/ui/routes/route-b/controller'; + + this.add('controller:route-a', Controller.extend({ + myService: inject.service('my-service', { source: routeASource }) + })); + + this.add('controller:route-b', Controller.extend({ + myService: inject.service('my-service', { source: routeBSource }) + })); + + let MyService = Service.extend(); + this.add({ + specifier: 'service:my-service', + source: routeASource + }, MyService); + + this.add({ + specifier: 'service:my-service', + source: routeBSource + }, MyService); + + return this.visit('/').then(() => { + let controllerA = this.applicationInstance.lookup('controller:route-a'); + let serviceFromControllerA = controllerA.get('myService'); + assert.ok(serviceFromControllerA instanceof MyService); + + let controllerB = this.applicationInstance.lookup('controller:route-a'); + assert.strictEqual(serviceFromControllerA, controllerB.get('myService')); + }); + } + + /* + * This test demonstrates a failure in the caching system of ember's + * container around singletons and and local lookup. The local lookup + * is cached and the global injection is then looked up incorrectly. + * + * The paractical rules of Ember's module unification config are such + * that services cannot be locally looked up, thus this case is really + * just a demonstration of what could go wrong if we permit arbitrary + * configuration (such as a singleton type that has local lookup). + */ + ['@skip Services can be injected with same name, different source, but same resolution result, and share an instance'](assert) { + // This test implies that there is a file src/ui/routes/route-a/-services/my-service + let routeASource = 'src/ui/routes/route-a/controller'; + let routeBSource = 'src/ui/routes/route-b/controller'; + + this.add('controller:route-a', Controller.extend({ + myService: inject.service('my-service', { source: routeASource }) + })); + + this.add('controller:route-b', Controller.extend({ + myService: inject.service('my-service', { source: routeBSource }) + })); + + let LocalLookupService = Service.extend(); + this.add({ + specifier: 'service:my-service', + source: routeASource + }, LocalLookupService); + + let MyService = Service.extend(); + this.add({ + specifier: 'service:my-service', + source: routeBSource + }, MyService); + + return this.visit('/').then(() => { + let controllerA = this.applicationInstance.lookup('controller:route-a'); + let serviceFromControllerA = controllerA.get('myService'); + assert.ok(serviceFromControllerA instanceof LocalLookupService, 'local lookup service is returned'); + + let controllerB = this.applicationInstance.lookup('controller:route-a'); + let serviceFromControllerB = controllerB.get('myService'); + assert.ok(serviceFromControllerB instanceof MyService, 'global service is returned'); + }); + } + + }); + +}