From 1d49ea9740a921ffef27c0fd08af09216f32e2e0 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Mon, 5 Apr 2021 23:22:45 -0700 Subject: [PATCH] fix: lookup/register and singleton flag behavior --- .../-internals/container/lib/container.ts | 7 +- .../container/tests/container_test.js | 244 ++++++++++++++---- 2 files changed, 191 insertions(+), 60 deletions(-) diff --git a/packages/@ember/-internals/container/lib/container.ts b/packages/@ember/-internals/container/lib/container.ts index 464cee719f6..0c26834bfdb 100644 --- a/packages/@ember/-internals/container/lib/container.ts +++ b/packages/@ember/-internals/container/lib/container.ts @@ -272,7 +272,7 @@ function isInstantiatable(container: Container, fullName: string) { function lookup(container: Container, fullName: string, options: LookupOptions = {}) { let normalizedName = fullName; - if (options.singleton !== false) { + if (options.singleton === true || (options.singleton === undefined && isSingleton(container, fullName))) { let cached = container.cache[normalizedName]; if (cached !== undefined) { return cached; @@ -335,7 +335,7 @@ function isSingletonInstance( return ( singleton !== false && instantiate !== false && - isSingleton(container, fullName) && + (singleton === true || isSingleton(container, fullName)) && isInstantiatable(container, fullName) ); } @@ -359,7 +359,7 @@ function isFactoryInstance( ) { return ( instantiate !== false && - (singleton !== false || isSingleton(container, fullName)) && + (singleton === false || !isSingleton(container, fullName)) && isInstantiatable(container, fullName) ); } @@ -379,6 +379,7 @@ function instantiateFactory( // SomeClass { singleton: true, instantiate: true } | { singleton: true } | { instantiate: true } | {} // By default majority of objects fall into this case if (isSingletonInstance(container, fullName, options)) { + let instance = (container.cache[normalizedName] = factoryManager.create() as CacheMember); // if this lookup happened _during_ destruction (emits a deprecation, but diff --git a/packages/@ember/-internals/container/tests/container_test.js b/packages/@ember/-internals/container/tests/container_test.js index f97ebbb6204..787819b3ba4 100644 --- a/packages/@ember/-internals/container/tests/container_test.js +++ b/packages/@ember/-internals/container/tests/container_test.js @@ -5,6 +5,193 @@ import { DEBUG } from '@glimmer/env'; import { Registry } from '..'; import { factory, moduleFor, AbstractTestCase, runTask } from 'internal-test-helpers'; +moduleFor('Container.lookup', class extends AbstractTestCase { + + ['@test lookup returns a fresh instance if singleton: false is passed as an option']( + assert + ) { + let registry = new Registry(); + let container = registry.container(); + let PostController = factory(); + + registry.register('controller:post', PostController); + + let postController1 = container.lookup('controller:post'); + let postController2 = container.lookup('controller:post', { + singleton: false, + }); + let postController3 = container.lookup('controller:post', { + singleton: false, + }); + let postController4 = container.lookup('controller:post'); + + assert.equal( + postController1.toString(), + postController4.toString(), + 'Singleton factories looked up normally return the same value' + ); + assert.notEqual( + postController1.toString(), + postController2.toString(), + 'Singleton factories are not equal to factories looked up with singleton: false' + ); + assert.notEqual( + postController2.toString(), + postController3.toString(), + 'Two factories looked up with singleton: false are not equal' + ); + assert.notEqual( + postController3.toString(), + postController4.toString(), + 'A singleton factory looked up after a factory called with singleton: false is not equal' + ); + + assert.ok( + postController1 instanceof PostController, + 'All instances are instances of the registered factory' + ); + assert.ok( + postController2 instanceof PostController, + 'All instances are instances of the registered factory' + ); + assert.ok( + postController3 instanceof PostController, + 'All instances are instances of the registered factory' + ); + assert.ok( + postController4 instanceof PostController, + 'All instances are instances of the registered factory' + ); + } + + ['@test lookup returns a fresh instance if singleton: false is passed as an option to lookup']( + assert + ) { + class TestFactory { + constructor(opts) { + Object.assign(this, opts); + } + static create(opts) { + return new this(opts); + } + } + + let registry = new Registry(); + let container = registry.container(); + registry.register('thing:test/obj', TestFactory); + + let instance1 = container.lookup('thing:test/obj'); + let instance2 = container.lookup('thing:test/obj', { + singleton: false, + }); + let instance3 = container.lookup('thing:test/obj', { + singleton: false, + }); + let instance4 = container.lookup('thing:test/obj'); + + assert.ok(instance1 === instance4, 'factories looked up up without singleton: false are the same instance'); + assert.ok(instance1 !== instance2, 'factories looked up with singleton: false are a different instance'); + assert.ok(instance2 !== instance3, 'factories looked up with singleton: false are a different instance'); + assert.ok(instance3 !== instance4, 'factories looked up after a call to singleton: false is a different instance'); + assert.ok( + instance1 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + assert.ok( + instance2 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + assert.ok( + instance3 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + assert.ok( + instance4 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + } + + ['@test lookup returns a fresh instance if singleton: false is passed as an option to register']( + assert + ) { + class TestFactory { + constructor(opts) { + Object.assign(this, opts); + } + static create(opts) { + return new this(opts); + } + } + + let registry = new Registry(); + let container = registry.container(); + registry.register('thing:test/obj', TestFactory, { singleton: false }); + + let instance1 = container.lookup('thing:test/obj'); + let instance2 = container.lookup('thing:test/obj'); + let instance3 = container.lookup('thing:test/obj'); + + assert.ok(instance1 !== instance2, 'each lookup is a different instance'); + assert.ok(instance2 !== instance3, 'each lookup is a different instance'); + assert.ok(instance1 !== instance3, 'each lookup is a different instance'); + assert.ok( + instance1 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + assert.ok( + instance2 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + assert.ok( + instance3 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + } + + ['@test lookup returns a singleton instance if singleton: true is passed as an option even if registered as singleton: false']( + assert + ) { + class TestFactory { + constructor(opts) { + Object.assign(this, opts); + } + static create(opts) { + return new this(opts); + } + } + + let registry = new Registry(); + let container = registry.container(); + registry.register('thing:test/obj', TestFactory, { singleton: false }); + + let instance1 = container.lookup('thing:test/obj'); + let instance2 = container.lookup('thing:test/obj', { singleton: true }); + let instance3 = container.lookup('thing:test/obj', { singleton: true }); + let instance4 = container.lookup('thing:test/obj'); + + assert.ok(instance1 !== instance2, 'each lookup is a different instance'); + assert.ok(instance2 === instance3, 'each singleton: true lookup is the same instance'); + assert.ok(instance3 !== instance4, 'each lookup is a different instance'); + assert.ok(instance1 !== instance4, 'each lookup is a different instance'); + assert.ok( + instance1 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + assert.ok( + instance2 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + assert.ok( + instance3 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + assert.ok( + instance4 instanceof TestFactory, + 'All instances are instances of the registered factory' + ); + } +}); + moduleFor( 'Container', class extends AbstractTestCase { @@ -45,63 +232,6 @@ moduleFor( ); } - ['@test A registered factory returns a fresh instance if singleton: false is passed as an option']( - assert - ) { - let registry = new Registry(); - let container = registry.container(); - let PostController = factory(); - - registry.register('controller:post', PostController); - - let postController1 = container.lookup('controller:post'); - let postController2 = container.lookup('controller:post', { - singleton: false, - }); - let postController3 = container.lookup('controller:post', { - singleton: false, - }); - let postController4 = container.lookup('controller:post'); - - assert.equal( - postController1.toString(), - postController4.toString(), - 'Singleton factories looked up normally return the same value' - ); - assert.notEqual( - postController1.toString(), - postController2.toString(), - 'Singleton factories are not equal to factories looked up with singleton: false' - ); - assert.notEqual( - postController2.toString(), - postController3.toString(), - 'Two factories looked up with singleton: false are not equal' - ); - assert.notEqual( - postController3.toString(), - postController4.toString(), - 'A singleton factory looked up after a factory called with singleton: false is not equal' - ); - - assert.ok( - postController1 instanceof PostController, - 'All instances are instances of the registered factory' - ); - assert.ok( - postController2 instanceof PostController, - 'All instances are instances of the registered factory' - ); - assert.ok( - postController3 instanceof PostController, - 'All instances are instances of the registered factory' - ); - assert.ok( - postController4 instanceof PostController, - 'All instances are instances of the registered factory' - ); - } - ["@test A factory type with a registered injection's instances receive that injection"]( assert ) {