From 79026a7a645021ace0cdf1cd3971ac4df3877430 Mon Sep 17 00:00:00 2001 From: Dan Gebhardt Date: Mon, 22 Aug 2016 15:00:50 -0400 Subject: [PATCH 1/3] [BUGFIX beta] Clone additional parent dependencies into engine instances. An engine instance needs to share the following with its parent: * `service:-glimmer-environment` (only for glimmer) * `renderer:-dom` or `renderer:-inert`, depending on whether the environment is interactive --- .../lib/system/application-instance.js | 1 + .../lib/system/engine-instance.js | 21 ++++++++++++++----- .../tests/system/application_instance_test.js | 21 ++++++++++++++----- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/packages/ember-application/lib/system/application-instance.js b/packages/ember-application/lib/system/application-instance.js index 7cc466585a5..295438c0b4f 100644 --- a/packages/ember-application/lib/system/application-instance.js +++ b/packages/ember-application/lib/system/application-instance.js @@ -484,6 +484,7 @@ BootOptions.prototype.toEnvironment = function() { let env = assign({}, environment); // For compatibility with existing code env.hasDOM = this.isBrowser; + env.isInteractive = this.isInteractive; env.options = this; return env; }; diff --git a/packages/ember-application/lib/system/engine-instance.js b/packages/ember-application/lib/system/engine-instance.js index 64c0ea29fcd..37f8e6e22b5 100644 --- a/packages/ember-application/lib/system/engine-instance.js +++ b/packages/ember-application/lib/system/engine-instance.js @@ -170,18 +170,29 @@ if (isEnabled('ember-application-engines')) { cloneParentDependencies() { let parent = getEngineParent(this); - [ + let registrations = [ 'route:basic', 'event_dispatcher:main', 'service:-routing' - ].forEach(key => this.register(key, parent.resolveRegistration(key))); + ]; - [ + if (isEnabled('ember-glimmer')) { + registrations.push('service:-glimmer-environment'); + } + + registrations.forEach(key => this.register(key, parent.resolveRegistration(key))); + + let env = parent.lookup('-environment:main'); + this.register('-environment:main', env, { instantiate: false }); + + let singletons = [ 'router:main', P`-bucket-cache:main`, '-view-registry:main', - '-environment:main' - ].forEach(key => this.register(key, parent.lookup(key), { instantiate: false })); + `renderer:-${env.isInteractive ? 'dom' : 'inert'}` + ]; + + singletons.forEach(key => this.register(key, parent.lookup(key), { instantiate: false })); this.inject('view', '_environment', '-environment:main'); this.inject('route', '_environment', '-environment:main'); diff --git a/packages/ember-application/tests/system/application_instance_test.js b/packages/ember-application/tests/system/application_instance_test.js index 60ae70a150b..5a00c313c80 100644 --- a/packages/ember-application/tests/system/application_instance_test.js +++ b/packages/ember-application/tests/system/application_instance_test.js @@ -141,7 +141,7 @@ QUnit.test('unregistering a factory clears all cached instances of that factory' if (isEnabled('ember-application-engines')) { QUnit.test('can build and boot a registered engine', function(assert) { - assert.expect(8); + assert.expect(isEnabled('ember-glimmer') ? 10 : 9); let ChatEngine = Engine.extend(); let chatEngineInstance; @@ -158,23 +158,34 @@ if (isEnabled('ember-application-engines')) { .then(() => { assert.ok(true, 'boot successful'); - [ + let registrations = [ 'route:basic', 'event_dispatcher:main', 'service:-routing' - ].forEach(key => { + ]; + + if (isEnabled('ember-glimmer')) { + registrations.push('service:-glimmer-environment'); + } + + registrations.forEach(key => { assert.strictEqual( chatEngineInstance.resolveRegistration(key), appInstance.resolveRegistration(key), `Engine and parent app share registrations for '${key}'`); }); - [ + let singletons = [ 'router:main', P`-bucket-cache:main`, '-view-registry:main', '-environment:main' - ].forEach(key => { + ]; + + let env = appInstance.lookup('-environment:main'); + singletons.push(env.isInteractive ? 'renderer:-dom' : 'renderer:-inert'); + + singletons.forEach(key => { assert.strictEqual( chatEngineInstance.lookup(key), appInstance.lookup(key), From 447df33c4db30c475a08415196dbb7012f08cc07 Mon Sep 17 00:00:00 2001 From: asakusuma Date: Wed, 17 Aug 2016 13:30:54 -0700 Subject: [PATCH 2/3] [BUGFIX beta] Fix basic component and helper resolution in engines. Ensure that Cache lookups are performed using the correct owner. Ensure that engines are destroyed _before_ top-level views so that top-level views are not re-instantiated during engine teardown. --- .../tests/system/visit_test.js | 70 +++++++++++++++++++ packages/ember-glimmer/lib/environment.js | 38 ++++++---- .../ember-glimmer/lib/helpers/component.js | 5 +- packages/ember-glimmer/lib/template.js | 8 +++ packages/ember-routing/lib/system/router.js | 12 ++-- 5 files changed, 110 insertions(+), 23 deletions(-) diff --git a/packages/ember-application/tests/system/visit_test.js b/packages/ember-application/tests/system/visit_test.js index f8706306a29..64a9150eda1 100644 --- a/packages/ember-application/tests/system/visit_test.js +++ b/packages/ember-application/tests/system/visit_test.js @@ -9,6 +9,7 @@ import Route from 'ember-routing/system/route'; import Router from 'ember-routing/system/router'; import Component from 'ember-templates/component'; import { compile } from 'ember-template-compiler/tests/utils/helpers'; +import { helper } from 'ember-templates/helper'; import jQuery from 'ember-views/system/jquery'; let App = null; @@ -386,6 +387,75 @@ QUnit.test('visit() returns a promise that resolves without rendering when shoul }); }); +QUnit.test('visit() on engine resolves engine component', function(assert) { + assert.expect(2); + + run(() => { + createApplication(); + + // Register engine + let BlogEngine = Engine.extend({ + init(...args) { + this._super.apply(this, args); + this.register('template:application', compile('{{cache-money}}')); + this.register('template:components/cache-money', compile(` +

Dis cache money

+ `)); + this.register('component:cache-money', Component.extend({})); + } + }); + App.register('engine:blog', BlogEngine); + + // Register engine route map + let BlogMap = function() {}; + App.register('route-map:blog', BlogMap); + + App.Router.map(function() { + this.mount('blog'); + }); + }); + + assert.strictEqual(jQuery('#qunit-fixture').children().length, 0, 'there are no elements in the fixture element'); + + return run(App, 'visit', '/blog', { shouldRender: true }).then(instance => { + assert.strictEqual(jQuery('#qunit-fixture').find('p').text(), 'Dis cache money', 'Engine component is resolved'); + }); +}); + +QUnit.test('visit() on engine resolves engine helper', function(assert) { + assert.expect(2); + + run(() => { + createApplication(); + + // Register engine + let BlogEngine = Engine.extend({ + init(...args) { + this._super.apply(this, args); + this.register('template:application', compile('{{swag}}')); + this.register('helper:swag', helper(function() { + return 'turnt up'; + })); + } + }); + App.register('engine:blog', BlogEngine); + + // Register engine route map + let BlogMap = function() {}; + App.register('route-map:blog', BlogMap); + + App.Router.map(function() { + this.mount('blog'); + }); + }); + + assert.strictEqual(jQuery('#qunit-fixture').children().length, 0, 'there are no elements in the fixture element'); + + return run(App, 'visit', '/blog', { shouldRender: true }).then(instance => { + assert.strictEqual(jQuery('#qunit-fixture').text(), 'turnt up', 'Engine component is resolved'); + }); +}); + QUnit.module('Ember.Application - visit() Integration Tests', { teardown() { if (instances) { diff --git a/packages/ember-glimmer/lib/environment.js b/packages/ember-glimmer/lib/environment.js index 6b0148b22f7..bb39fc564c9 100644 --- a/packages/ember-glimmer/lib/environment.js +++ b/packages/ember-glimmer/lib/environment.js @@ -60,7 +60,7 @@ function buildTextFieldSyntax({ args, templates }, getDefinition) { } const builtInDynamicComponents = { - input({ key, args, templates }, getDefinition) { + input({ key, args, templates }, symbolTable, getDefinition) { if (args.named.has('type')) { let typeArg = args.named.at('type'); if (typeArg.type === 'value') { @@ -79,7 +79,7 @@ const builtInDynamicComponents = { } else { return buildTextFieldSyntax({ args, templates }, getDefinition); } - return DynamicComponentSyntax.create({ args, templates }); + return DynamicComponentSyntax.create({ args, templates, symbolTable }); } }; @@ -134,12 +134,12 @@ export default class Environment extends GlimmerEnvironment { this.uselessAnchor = document.createElement('a'); - this._definitionCache = new Cache(2000, ({ name, source }) => { + this._definitionCache = new Cache(2000, ({ name, source, owner }) => { let { component: ComponentClass, layout } = lookupComponent(owner, name, { source }); if (ComponentClass || layout) { return new CurlyComponentDefinition(name, ComponentClass, layout); } - }, ({ name, source }) => { + }, ({ name, source, owner }) => { return source && owner._resolveLocalLookupName(name, source) || name; }); @@ -241,7 +241,7 @@ export default class Environment extends GlimmerEnvironment { let generateBuiltInSyntax = builtInDynamicComponents[key]; if (generateBuiltInSyntax) { - return generateBuiltInSyntax(statement, (path) => this.getComponentDefinition([path], symbolTable)); + return generateBuiltInSyntax(statement, symbolTable, (path) => this.getComponentDefinition([path], symbolTable)); } assert(`A helper named "${key}" could not be found`, !isBlock || this.hasHelper(key, symbolTable)); @@ -266,8 +266,11 @@ export default class Environment extends GlimmerEnvironment { getComponentDefinition(path, symbolTable) { let name = path[0]; - let source = symbolTable && `template:${symbolTable.getMeta().moduleName}`; - return this._definitionCache.get({ name, source }); + let blockMeta = symbolTable.getMeta(); + let owner = blockMeta.owner; + let source = `template:${blockMeta.moduleName}`; + + return this._definitionCache.get({ name, source, owner }); } // normally templates should be exported at the proper module name @@ -300,20 +303,27 @@ export default class Environment extends GlimmerEnvironment { } hasHelper(name, symbolTable) { - let options = symbolTable && { source: `template:${symbolTable.getMeta().moduleName}` } || {}; + let blockMeta = symbolTable.getMeta(); + let owner = blockMeta.owner; + let options = { source: `template:${blockMeta.moduleName}` }; + return !!builtInHelpers[name[0]] || - this.owner.hasRegistration(`helper:${name}`, options) || - this.owner.hasRegistration(`helper:${name}`); + owner.hasRegistration(`helper:${name}`, options) || + owner.hasRegistration(`helper:${name}`); } lookupHelper(name, symbolTable) { - let options = symbolTable && { source: `template:${symbolTable.getMeta().moduleName}` } || {}; + let blockMeta = symbolTable.getMeta(); + let owner = blockMeta.owner; + let options = blockMeta.moduleName && { source: `template:${blockMeta.moduleName}` } || {}; + let helper = builtInHelpers[name[0]] || - this.owner.lookup(`helper:${name}`, options) || - this.owner.lookup(`helper:${name}`); + owner.lookup(`helper:${name}`, options) || + owner.lookup(`helper:${name}`); + // TODO: try to unify this into a consistent protocol to avoid wasteful closure allocations if (helper.isInternalHelper) { - return (vm, args) => helper.toReference(args, this); + return (vm, args) => helper.toReference(args, this, symbolTable); } else if (helper.isHelperInstance) { return (vm, args) => SimpleHelperReference.create(helper.compute, args); } else if (helper.isHelperFactory) { diff --git a/packages/ember-glimmer/lib/helpers/component.js b/packages/ember-glimmer/lib/helpers/component.js index 6ef22fb2bc7..02811f07a4b 100644 --- a/packages/ember-glimmer/lib/helpers/component.js +++ b/packages/ember-glimmer/lib/helpers/component.js @@ -126,8 +126,7 @@ function curryArgs(definition, newArgs) { export default { isInternalHelper: true, - toReference(args, env) { - // TODO: Need to figure out what to do about symbolTable here. - return ClosureComponentReference.create(args, null, env); + toReference(args, env, symbolTable) { + return ClosureComponentReference.create(args, symbolTable, env); } }; diff --git a/packages/ember-glimmer/lib/template.js b/packages/ember-glimmer/lib/template.js index d090ee8d70c..b0e97e7dcf9 100644 --- a/packages/ember-glimmer/lib/template.js +++ b/packages/ember-glimmer/lib/template.js @@ -2,6 +2,14 @@ import { Template } from 'glimmer-runtime'; class Wrapper { constructor(id, env, spec) { + let { owner } = env; + if (spec.meta) { + spec.meta.owner = owner; + } else { + spec.meta = { + owner + }; + } this.id = id; this.env = env; this.spec = spec; diff --git a/packages/ember-routing/lib/system/router.js b/packages/ember-routing/lib/system/router.js index 1fc99102327..ba64466db99 100644 --- a/packages/ember-routing/lib/system/router.js +++ b/packages/ember-routing/lib/system/router.js @@ -455,12 +455,6 @@ const EmberRouter = EmberObject.extend(Evented, { }, willDestroy() { - if (this._toplevelView) { - this._toplevelView.destroy(); - this._toplevelView = null; - } - this._super(...arguments); - if (isEnabled('ember-application-engines')) { let instances = this._engineInstances; for (let name in instances) { @@ -470,6 +464,12 @@ const EmberRouter = EmberObject.extend(Evented, { } } + if (this._toplevelView) { + this._toplevelView.destroy(); + this._toplevelView = null; + } + this._super(...arguments); + this.reset(); }, From 02814c2da26a1558fb06eb527ef83e829dae7016 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Fri, 26 Aug 2016 18:54:27 -0400 Subject: [PATCH 3/3] Use the correct owner for each template lookup. Ensure that the owner being used is not the environments default `Owner`, but it is the owner that the template is being looked up by. For late bound templates this means the components owner, and for normal templates (looked up from registry) it is passed to the factories `.create` method by the container itself. --- .../lib/system/engine-instance.js | 3 + packages/ember-glimmer/lib/environment.js | 20 ++++--- .../lib/syntax/curly-component.js | 5 +- packages/ember-glimmer/lib/template.js | 11 ++-- .../integration/application/engine-test.js | 60 +++++++++++++++++++ .../tests/unit/layout-cache-test.js | 2 +- .../tests/unit/template-factory-test.js | 4 +- .../tests/utils/abstract-test-case.js | 4 ++ 8 files changed, 92 insertions(+), 17 deletions(-) create mode 100644 packages/ember-glimmer/tests/integration/application/engine-test.js diff --git a/packages/ember-application/lib/system/engine-instance.js b/packages/ember-application/lib/system/engine-instance.js index 37f8e6e22b5..6475f642650 100644 --- a/packages/ember-application/lib/system/engine-instance.js +++ b/packages/ember-application/lib/system/engine-instance.js @@ -13,6 +13,7 @@ import { getEngineParent, setEngineParent } from 'ember-application/system/engin import { assert } from 'ember-metal/debug'; import run from 'ember-metal/run_loop'; import RSVP from 'ember-runtime/ext/rsvp'; +import { guidFor } from 'ember-metal/utils'; import isEnabled from 'ember-metal/features'; /** @@ -39,6 +40,8 @@ const EngineInstance = EmberObject.extend(RegistryProxy, ContainerProxy, { init() { this._super(...arguments); + guidFor(this); + let base = this.base; if (!base) { diff --git a/packages/ember-glimmer/lib/environment.js b/packages/ember-glimmer/lib/environment.js index bb39fc564c9..0eefb4d8276 100644 --- a/packages/ember-glimmer/lib/environment.js +++ b/packages/ember-glimmer/lib/environment.js @@ -1,3 +1,4 @@ +import { guidFor } from 'ember-metal/utils'; import lookupPartial, { hasPartial } from 'ember-views/system/lookup_partial'; import { Environment as GlimmerEnvironment, @@ -140,12 +141,15 @@ export default class Environment extends GlimmerEnvironment { return new CurlyComponentDefinition(name, ComponentClass, layout); } }, ({ name, source, owner }) => { - return source && owner._resolveLocalLookupName(name, source) || name; + let expandedName = source && owner._resolveLocalLookupName(name, source) || name; + let ownerGuid = guidFor(owner); + + return ownerGuid + '|' + expandedName; }); - this._templateCache = new Cache(1000, Template => { - return Template.create({ env: this }); - }, template => template.id); + this._templateCache = new Cache(1000, ({ Template, owner }) => { + return Template.create({ env: this, [OWNER]: owner }); + }, ({ Template, owner }) => guidFor(owner) + '|' + Template.id); this._compilerCache = new Cache(10, Compiler => { return new Cache(2000, template => { @@ -276,14 +280,14 @@ export default class Environment extends GlimmerEnvironment { // normally templates should be exported at the proper module name // and cached in the container, but this cache supports templates // that have been set directly on the component's layout property - getTemplate(Template) { - return this._templateCache.get(Template); + getTemplate(Template, owner) { + return this._templateCache.get({ Template, owner }); } // a Compiler can wrap the template so it needs its own cache - getCompiledBlock(Compiler, template) { + getCompiledBlock(Compiler, template, owner) { let compilerCache = this._compilerCache.get(Compiler); - return compilerCache.get(template); + return compilerCache.get(template, owner); } hasPartial(name) { diff --git a/packages/ember-glimmer/lib/syntax/curly-component.js b/packages/ember-glimmer/lib/syntax/curly-component.js index fc7e7fb2968..44c905b5435 100644 --- a/packages/ember-glimmer/lib/syntax/curly-component.js +++ b/packages/ember-glimmer/lib/syntax/curly-component.js @@ -9,6 +9,7 @@ import get from 'ember-metal/property_get'; import { _instrumentStart } from 'ember-metal/instrumentation'; import { ComponentDefinition } from 'glimmer-runtime'; import Component from '../component'; +import { OWNER } from 'container/owner'; const DEFAULT_LAYOUT = P`template:components/-default`; @@ -230,10 +231,10 @@ class CurlyComponentManager { templateFor(component, env) { let Template = component.layout; + let owner = component[OWNER]; if (Template) { - return env.getTemplate(Template); + return env.getTemplate(Template, owner); } - let { owner } = env; let layoutName = get(component, 'layoutName'); if (layoutName) { let template = owner.lookup('template:' + layoutName); diff --git a/packages/ember-glimmer/lib/template.js b/packages/ember-glimmer/lib/template.js index b0e97e7dcf9..637a4ed6338 100644 --- a/packages/ember-glimmer/lib/template.js +++ b/packages/ember-glimmer/lib/template.js @@ -1,8 +1,8 @@ import { Template } from 'glimmer-runtime'; +import { OWNER } from 'container/owner'; class Wrapper { - constructor(id, env, spec) { - let { owner } = env; + constructor(id, env, owner, spec) { if (spec.meta) { spec.meta.owner = owner; } else { @@ -39,8 +39,11 @@ export default function template(json) { let id = ++templateId; return { id, - create({ env }) { - return new Wrapper(id, env, JSON.parse(json)); + create(options) { + let env = options.env; + let owner = options[OWNER]; + + return new Wrapper(id, env, owner, JSON.parse(json)); } }; } diff --git a/packages/ember-glimmer/tests/integration/application/engine-test.js b/packages/ember-glimmer/tests/integration/application/engine-test.js new file mode 100644 index 00000000000..c3bbcd40d03 --- /dev/null +++ b/packages/ember-glimmer/tests/integration/application/engine-test.js @@ -0,0 +1,60 @@ +import packageName from '../../utils/package-name'; +import { moduleFor, ApplicationTest } from '../../utils/test-case'; +import { strip } from '../../utils/abstract-test-case'; +import { compile } from '../../utils/helpers'; +import Controller from 'ember-runtime/controllers/controller'; +import Engine from 'ember-application/system/engine'; +import isEnabled from 'ember-metal/features'; + +// only run these tests for ember-glimmer when the feature is enabled, or for +// ember-htmlbars when the feature is not enabled +const shouldRun = isEnabled('ember-application-engines') && ( + ( + (isEnabled('ember-glimmer') && packageName === 'glimmer') || + (!isEnabled('ember-glimmer') && packageName === 'htmlbars') + ) +); + +if (shouldRun) { + moduleFor('Application test: engine rendering', class extends ApplicationTest { + ['@test sharing a template between engine and application has separate refinements']() { + this.assert.expect(1); + + let sharedTemplate = compile(strip` +

{{contextType}}

+ {{ambiguous-curlies}} + + {{outlet}} + `); + + this.application.register('template:application', sharedTemplate); + this.registerController('application', Controller.extend({ + contextType: 'Application', + 'ambiguous-curlies': 'Controller Data!' + })); + + this.router.map(function() { + this.mount('blog'); + }); + this.application.register('route-map:blog', function() { }); + + this.registerEngine('blog', Engine.extend({ + init() { + this._super(...arguments); + + this.register('controller:application', Controller.extend({ + contextType: 'Engine' + })); + this.register('template:application', sharedTemplate); + this.register('template:components/ambiguous-curlies', compile(strip` +

Component!

+ `)); + } + })); + + return this.visit('/blog').then(() => { + this.assertText('ApplicationController Data!EngineComponent!'); + }); + } + }); +} diff --git a/packages/ember-glimmer/tests/unit/layout-cache-test.js b/packages/ember-glimmer/tests/unit/layout-cache-test.js index 407f4ca6267..ef810751613 100644 --- a/packages/ember-glimmer/tests/unit/layout-cache-test.js +++ b/packages/ember-glimmer/tests/unit/layout-cache-test.js @@ -51,7 +51,7 @@ moduleFor('Layout cache test', class extends RenderingTest { templateFor(content) { let Factory = this.compile(content); - return this.env.getTemplate(Factory); + return this.env.getTemplate(Factory, this.owner); } ['@test each template is only compiled once'](assert) { diff --git a/packages/ember-glimmer/tests/unit/template-factory-test.js b/packages/ember-glimmer/tests/unit/template-factory-test.js index a836d5d78dc..0271df21d4c 100644 --- a/packages/ember-glimmer/tests/unit/template-factory-test.js +++ b/packages/ember-glimmer/tests/unit/template-factory-test.js @@ -28,12 +28,12 @@ moduleFor('Template factory test', class extends RenderingTest { assert.equal(env._templateCache.misses, 0, 'misses 0'); assert.equal(env._templateCache.hits, 0, 'hits 0'); - let precompiled = env.getTemplate(Precompiled); + let precompiled = env.getTemplate(Precompiled, env.owner); assert.equal(env._templateCache.misses, 1, 'misses 1'); assert.equal(env._templateCache.hits, 0, 'hits 0'); - let compiled = env.getTemplate(Compiled); + let compiled = env.getTemplate(Compiled, env.owner); assert.equal(env._templateCache.misses, 2, 'misses 2'); assert.equal(env._templateCache.hits, 0, 'hits 0'); diff --git a/packages/ember-glimmer/tests/utils/abstract-test-case.js b/packages/ember-glimmer/tests/utils/abstract-test-case.js index b7244372d68..346a559c7d8 100644 --- a/packages/ember-glimmer/tests/utils/abstract-test-case.js +++ b/packages/ember-glimmer/tests/utils/abstract-test-case.js @@ -307,6 +307,10 @@ export class AbstractApplicationTest extends TestCase { registerController(name, controller) { this.application.register(`controller:${name}`, controller); } + + registerEngine(name, engine) { + this.application.register(`engine:${name}`, engine); + } } export class AbstractRenderingTest extends TestCase {