From 32bde2d0bbb0f5f7465aa3e73011928b7621c824 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Sat, 6 Aug 2016 21:48:30 +0300 Subject: [PATCH] feat($injector): add support for non-string IDs Previously, only strings could be used as identifiers for Angular services. This commit adds support for using any value as identifier for an Angular service (e.g. used with `.provider()`, `.factory()`, `.service()`, `.value()`, `.constant()`, `.decorator()`). Among other things, non-string identifiers enable: - Private services (without relying on naming conventions). - Collision avoidance (without relying on custom prefixes and namespacing). - Better toolability (e.g. autocomplete support for service identifiers). - Better compression (i.e. strings can't be minified, non-string values could). Identifiers for directives and filters are still restricted to string values, since they need to be referenced in HTML (text). -- For services with string IDs, the corresponding provider ID is constructed by appending `Provider` to the service ID. For services with non-string IDs, the corresponding provider has the exact same ID (it is the context that determines if a service or a provider should be injected). E.g.: ```js var bar = {}; angular. module('myModule', []). provider('foo' /* string ID */, {$get: function () { return 'FOO'; }}). provider( bar /* non-string ID */, {$get: function () { return 'BAR'; }}). config(['fooProvider', function (fooProvider) { // `foo` provider injected (because we are in config block) }]). run(['foo', function (foo) { // `foo` service injected (because we are in run block) }]). config([bar, function (barProvider) { // `bar` provider injected (because we are in config block) // (even though we used the same identifier (`bar`) that we will use in the run block) }]). run([bar, function (bar) { // `bar` service injected (because we are in run block) }]); ``` -- This change is backwards compatible (afaict). Fixes #10347 --- docs/content/error/$injector/itkn.ngdoc | 26 ---- src/auto/injector.js | 113 ++++++++------ test/auto/injectorSpec.js | 187 ++++++++++++++++++++++++ 3 files changed, 254 insertions(+), 72 deletions(-) delete mode 100644 docs/content/error/$injector/itkn.ngdoc diff --git a/docs/content/error/$injector/itkn.ngdoc b/docs/content/error/$injector/itkn.ngdoc deleted file mode 100644 index dcae73b9b57b..000000000000 --- a/docs/content/error/$injector/itkn.ngdoc +++ /dev/null @@ -1,26 +0,0 @@ -@ngdoc error -@name $injector:itkn -@fullName Bad Injection Token -@description - -This error occurs when using a bad token as a dependency injection annotation. -Dependency injection annotation tokens should always be strings. Using any other -type will cause this error to be thrown. - -Examples of code with bad injection tokens include: - -``` -var myCtrl = function ($scope, $http) { /* ... */ }; -myCtrl.$inject = ['$scope', 42]; - -myAppModule.controller('MyCtrl', ['$scope', {}, function ($scope, $timeout) { - // ... -}]); -``` - -The bad injection tokens are `42` in the first example and `{}` in the second. -To avoid the error, always use string literals for dependency injection annotation -tokens. - -For an explanation of what injection annotations are and how to use them, refer -to the {@link guide/di Dependency Injection Guide}. diff --git a/src/auto/injector.js b/src/auto/injector.js index c0feb62d443f..b558ee1e53a0 100644 --- a/src/auto/injector.js +++ b/src/auto/injector.js @@ -63,6 +63,7 @@ * Implicit module which gets automatically added to each {@link auto.$injector $injector}. */ +var PROVIDER_ID_SUFFIX = 'Provider'; var ARROW_ARG = /^([^\(]+?)=>/; var FN_ARGS = /^[^\(]*\(\s*([^\)]*)\)/m; var FN_ARG_SPLIT = /,/; @@ -129,6 +130,14 @@ function annotate(fn, strictDi, name) { return $inject; } +function getProviderId(id) { + return !isString(id) ? id : id + PROVIDER_ID_SUFFIX; +} + +function stringifyIdForError(id, suffix) { + return isString(id) ? id : id + suffix; +} + /////////////////////////////////////// /** @@ -647,10 +656,9 @@ function annotate(fn, strictDi, name) { function createInjector(modulesToLoad, strictDi) { strictDi = (strictDi === true); var INSTANTIATING = {}, - providerSuffix = 'Provider', path = [], loadedModules = new HashMap(null, true), - providerCache = { + providerCache = new HashMap({ $provide: { provider: supportObject(provider), factory: supportObject(factory), @@ -659,24 +667,22 @@ function createInjector(modulesToLoad, strictDi) { constant: supportObject(constant), decorator: decorator } - }, - providerInjector = (providerCache.$injector = - createInternalInjector(providerCache, function(serviceName, caller) { - if (angular.isString(caller)) { - path.push(caller); - } + }), + instanceCache = new HashMap(), + providerInjector = + createInternalInjector(providerCache, function(serviceName) { throw $injectorMinErr('unpr', "Unknown provider: {0}", path.join(' <- ')); - })), - instanceCache = {}, + }, ' (provider)'), protoInstanceInjector = - createInternalInjector(instanceCache, function(serviceName, caller) { - var provider = providerInjector.get(serviceName + providerSuffix, caller); - return instanceInjector.invoke( - provider.$get, provider, undefined, serviceName); + createInternalInjector(instanceCache, function(serviceName) { + var provider = providerInjector.get(getProviderId(serviceName)); + return instanceInjector.invoke(provider.$get, provider); }), instanceInjector = protoInstanceInjector; - providerCache['$injector' + providerSuffix] = { $get: valueFn(protoInstanceInjector) }; + providerCache.put('$injector', providerInjector); + providerCache.put(getProviderId('$injector'), {$get: valueFn(protoInstanceInjector)}); + var runBlocks = loadModules(modulesToLoad); instanceInjector = protoInstanceInjector.get('$injector'); instanceInjector.strictDi = strictDi; @@ -690,7 +696,7 @@ function createInjector(modulesToLoad, strictDi) { function supportObject(delegate) { return function(key, value) { - if (isObject(key)) { + if ((arguments.length === 1) && isObject(key)) { forEach(key, reverseParams(delegate)); } else { return delegate(key, value); @@ -706,7 +712,10 @@ function createInjector(modulesToLoad, strictDi) { if (!provider_.$get) { throw $injectorMinErr('pget', "Provider '{0}' must define $get factory method.", name); } - return (providerCache[name + providerSuffix] = provider_); + + providerCache.put(getProviderId(name), provider_); + + return provider_; } function enforceReturnValue(name, factory) { @@ -737,12 +746,12 @@ function createInjector(modulesToLoad, strictDi) { function constant(name, value) { assertNotHasOwnProperty(name, 'constant'); - providerCache[name] = value; - instanceCache[name] = value; + providerCache.put(name, value); + instanceCache.put(name, value); } function decorator(serviceName, decorFn) { - var origProvider = providerInjector.get(serviceName + providerSuffix), + var origProvider = providerInjector.get(getProviderId(serviceName)), orig$get = origProvider.$get; origProvider.$get = function() { @@ -805,29 +814,45 @@ function createInjector(modulesToLoad, strictDi) { // internal Injector //////////////////////////////////// - function createInternalInjector(cache, factory) { + function createInternalInjector(cache, factory, displayNameSuffix) { + if (!isString(displayNameSuffix)) { + displayNameSuffix = ''; + } function getService(serviceName, caller) { - if (cache.hasOwnProperty(serviceName)) { - if (cache[serviceName] === INSTANTIATING) { - throw $injectorMinErr('cdep', 'Circular dependency found: {0}', - serviceName + ' <- ' + path.join(' <- ')); - } - return cache[serviceName]; - } else { - try { - path.unshift(serviceName); - cache[serviceName] = INSTANTIATING; - cache[serviceName] = factory(serviceName, caller); - return cache[serviceName]; - } catch (err) { - if (cache[serviceName] === INSTANTIATING) { - delete cache[serviceName]; + var hasCaller = isDefined(caller); + var hadInstance = cache.has(serviceName); + var instance; + + if (hasCaller) { + path.unshift(stringifyIdForError(caller, displayNameSuffix)); + } + path.unshift(stringifyIdForError(serviceName, displayNameSuffix)); + + try { + if (hadInstance) { + instance = cache.get(serviceName); + + if (instance === INSTANTIATING) { + throw $injectorMinErr('cdep', 'Circular dependency found: {0}', path.join(' <- ')); } - throw err; - } finally { - path.shift(); + + return instance; + } else { + cache.put(serviceName, INSTANTIATING); + + instance = factory(serviceName); + cache.put(serviceName, instance); + + return instance; + } + } finally { + if (!hadInstance && (cache.get(serviceName) === INSTANTIATING)) { + cache.remove(serviceName); } + + path.shift(); + if (hasCaller) path.shift(); } } @@ -838,12 +863,8 @@ function createInjector(modulesToLoad, strictDi) { for (var i = 0, length = $inject.length; i < length; i++) { var key = $inject[i]; - if (typeof key !== 'string') { - throw $injectorMinErr('itkn', - 'Incorrect injection token! Expected service name as string, got {0}', key); - } - args.push(locals && locals.hasOwnProperty(key) ? locals[key] : - getService(key, serviceName)); + var localsHasKey = locals && isString(key) && locals.hasOwnProperty(key); + args.push(localsHasKey ? locals[key] : getService(key, serviceName)); } return args; } @@ -901,7 +922,7 @@ function createInjector(modulesToLoad, strictDi) { get: getService, annotate: createInjector.$$annotate, has: function(name) { - return providerCache.hasOwnProperty(name + providerSuffix) || cache.hasOwnProperty(name); + return cache.has(name) || providerCache.has(getProviderId(name)); } }; } diff --git a/test/auto/injectorSpec.js b/test/auto/injectorSpec.js index 77d2a490418e..ff483e25ea40 100644 --- a/test/auto/injectorSpec.js +++ b/test/auto/injectorSpec.js @@ -1168,3 +1168,190 @@ describe('strict-di injector', function() { expect($injector.strictDi).toBe(true); })); }); + +describe('injector with non-string IDs', function() { + it('should support non-string service identifiers', function() { + var ids = [ + null, + true, + 1, + {}, + [], + noop, + /./ + ]; + + module(function($provide) { + ids.forEach(function(id, idx) { $provide.value(id, idx); }); + $provide.factory('allDeps', ids.concat(function() { return sliceArgs(arguments); })); + }); + + inject(function(allDeps) { + expect(allDeps.length).toBe(ids.length); + expect(allDeps.every(function(dep, idx) { return dep === idx; })).toBe(true); + }); + }); + + + it('should support configuring services with non-string identifiers', function() { + /* eslint-disable no-new-wrappers */ + var id1 = new String('same string, no problem'); + var id2 = new String('same string, no problem'); + /* eslint-enable */ + + angular. + module('test', []). + factory(id2, [id1, identity]). + provider(id1, function Id1Provider() { + var value = 'foo'; + this.setValue = function setValue(newValue) { value = newValue; }; + this.$get = function $get() { return value; }; + }). + config([id1, function config(id1Provider) { + id1Provider.setValue('bar'); + }]); + + module('test'); + inject([id2, function(dep2) { + expect(dep2).toBe('bar'); + }]); + }); + + + it('should support decorating services with non-string identifiers', function() { + /* eslint-disable no-new-wrappers */ + var id1 = new String('same string, no problem'); + var id2 = new String('same string, no problem'); + /* eslint-enable */ + + angular. + module('test', []). + factory(id2, [id1, identity]). + value(id1, 'foo'). + decorator(id1, function decorator($delegate) { + expect($delegate).toBe('foo'); + return 'bar'; + }); + + module('test'); + inject([id2, function(dep2) { + expect(dep2).toBe('bar'); + }]); + }); + + + it('should still allow passing multiple providers as object', function() { + var obj = { + foo: 'foo', + bar: 'bar' + }; + + module(function($provide) { + $provide.value(obj); + $provide.value(obj, 'foo&bar'); + }); + + inject(['foo', 'bar', obj, function(foo, bar, fooBar) { + expect(foo).toBe('foo'); + expect(bar).toBe('bar'); + expect(fooBar).toBe('foo&bar'); + }]); + }); + + + describe('should stringify non-string identifiers for error messages', function() { + var foo, bar, baz; + + beforeEach(function() { + foo = {toString: valueFn('fooThingy')}; + bar = {toString: valueFn('barThingy')}; + baz = {toString: valueFn('bazThingy')}; + }); + + + it('(Unknown provider)', function() { + var executed = false; + + module(function($provide) { + expect(function() { + $provide.provider('foo', ['barProvider', noop]); + }).toThrowMinErr('$injector', 'unpr', 'Unknown provider: barProvider\n'); + + expect(function() { + $provide.provider('foo', [{}, noop]); + }).toThrowMinErr('$injector', 'unpr', 'Unknown provider: [object Object] (provider)\n'); + + expect(function() { + $provide.provider('foo', [bar, noop]); + }).toThrowMinErr('$injector', 'unpr', 'Unknown provider: barThingy (provider)\n'); + + executed = true; + }); + + inject(); + + expect(executed).toBe(true); + }); + + + it('(Unknown service)', function() { + var executed = false; + + module(function($provide) { + $provide.provider(foo, valueFn({$get: ['bar', noop]})); + $provide.provider('bar', valueFn({$get: [baz, noop]})); + + $provide.provider('foo', valueFn({$get: [bar, noop]})); + $provide.provider(bar, valueFn({$get: ['baz', noop]})); + }); + + inject(function($injector) { + var specs = [ + ['bar', 'bazThingy (provider) <- bazThingy <- bar'], + [foo, 'bazThingy (provider) <- bazThingy <- bar <- fooThingy'], + [bar, 'bazProvider <- baz <- barThingy'], + ['foo', 'bazProvider <- baz <- barThingy <- foo'] + ]; + + forEach(specs, function(spec) { + var serviceId = spec[0]; + var errorPath = spec[1]; + + expect(function() { + $injector.get(serviceId); + }).toThrowMinErr('$injector', 'unpr', 'Unknown provider: ' + errorPath + '\n'); + }); + + executed = true; + }); + + expect(executed).toBe(true); + }); + + + it('(Circular dependency)', function() { + var executed = false; + + module(function($provide) { + $provide.provider(foo, valueFn({$get: ['bar', noop]})); + $provide.provider('bar', valueFn({$get: [baz, noop]})); + $provide.provider(baz, valueFn({$get: ['foo', noop]})); + $provide.provider('foo', valueFn({$get: [bar, noop]})); + $provide.provider(bar, valueFn({$get: ['baz', noop]})); + $provide.provider('baz', valueFn({$get: [foo, noop]})); + }); + + inject(function($injector) { + var errorPath = 'fooThingy <- baz <- barThingy <- foo <- bazThingy <- bar <- fooThingy'; + + expect(function() { + $injector.get(foo); + }).toThrowMinErr('$injector', 'cdep', 'Circular dependency found: ' + errorPath + '\n'); + + executed = true; + }); + + expect(executed).toBe(true); + }); + }); +});