Skip to content

Commit

Permalink
[BUGFIX release] Avoid creating event dispatcher in FastBoot with Eng…
Browse files Browse the repository at this point in the history
…ines

Prior to this commit, the `event_dispatcher:main` was always being
instantiated so that it can be injected eagerly into child engines. In
general, this is good (we should only ever have a single event
dispatcher going) but in the FastBoot case the event dispatcher should
not have been created at all (since there is no DOM or interactivity
booting the event dispatcher to listen to DOM events is wasteful and
error prone).

This commit updates the EventDispatcher base class to properly look for
the `isInteractive` flag (which is what it should have been doing all
along) on the `-environment:main` that is injected from the
`Application.visit` API. When `isInteractive` is falsey and the event
dispatcher is instantiated we will issue an assertion.

We also avoid eagerly looking up (and therefore forcing to initialize)
the EventDispatcher if `.visit` API is passed `isInteractive: false`.
This avoids the specifically reported issue of triggering an assertion
in FastBoot mode.

(cherry picked from commit 6e9a9c1)
  • Loading branch information
rwjblue committed Oct 4, 2017
1 parent 3aec03a commit 3727e2e
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 15 deletions.
5 changes: 4 additions & 1 deletion packages/ember-application/lib/system/engine-instance.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,12 @@ const EngineInstance = EmberObject.extend(RegistryProxyMixin, ContainerProxyMixi
'-view-registry:main',
`renderer:-${env.isInteractive ? 'dom' : 'inert'}`,
'service:-document',
'event_dispatcher:main'
];

if (env.isInteractive) {
singletons.push('event_dispatcher:main');
}

singletons.forEach(key => this.register(key, parent.lookup(key), { instantiate: false }));

this.inject('view', '_environment', '-environment:main');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ QUnit.test('customEvents added to the application before setupEventDispatcher',
assert.expect(1);

appInstance = run(() => ApplicationInstance.create({ application }));
appInstance.setupRegistry();

application.customEvents = {
awesome: 'sauce'
Expand All @@ -80,6 +81,7 @@ QUnit.test('customEvents added to the application before setupEventDispatcher',
assert.expect(1);

run(() => appInstance = ApplicationInstance.create({ application }));
appInstance.setupRegistry();

application.customEvents = {
awesome: 'sauce'
Expand All @@ -97,6 +99,7 @@ QUnit.test('customEvents added to the application instance before setupEventDisp
assert.expect(1);

appInstance = run(() => ApplicationInstance.create({ application }));
appInstance.setupRegistry();

appInstance.customEvents = {
awesome: 'sauce'
Expand Down
46 changes: 46 additions & 0 deletions packages/ember-application/tests/system/visit_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,52 @@ moduleFor('Ember.Application - visit()', class extends ApplicationTestCase {
});
}

[`@test visit() does not setup the event_dispatcher:main if isInteractive is false (with Engines) GH#15615`](assert) {
assert.expect(3);

this.router.map(function() {
this.mount('blog');
});

this.addTemplate('application', '<h1>Hello world</h1>{{outlet}}');
this.add('event_dispatcher:main', {
create() { throw new Error('should not happen!'); }
});

// 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(`
<p>Dis cache money</p>
`));
this.register('component:cache-money', Component.extend({}));
}
});
this.add('engine:blog', BlogEngine);

// Register engine route map
let BlogMap = function() {};
this.add('route-map:blog', BlogMap);

assert.strictEqual(
this.$('#qunit-fixture').children().length, 0,
'there are no elements in the fixture element'
);

return this.visit('/blog', { isInteractive: false }).then(instance => {
assert.ok(
instance instanceof ApplicationInstance,
'promise is resolved with an ApplicationInstance'
);
assert.strictEqual(
this.$().find('p').text(), 'Dis cache money',
'Engine component is resolved'
);
});
}

[`@test visit() on engine resolves engine component`](assert) {
assert.expect(2);

Expand Down
30 changes: 21 additions & 9 deletions packages/ember-glimmer/tests/integration/event-dispatcher-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,15 +209,6 @@ moduleFor('EventDispatcher#setup', class extends RenderingTest {
this.render(`{{x-foo}}`);
}

['@test canDispatchToEventManager is deprecated in EventDispatcher'](assert) {
let MyDispatcher = EventDispatcher.extend({
canDispatchToEventManager: null
});

expectDeprecation(/`canDispatchToEventManager` has been deprecated/);
MyDispatcher.create();
}

['@test a rootElement can be specified'](assert) {
this.$().append('<div id="app"></div>');
this.dispatcher.setup({ myevent: 'myEvent' }, '#app');
Expand Down Expand Up @@ -260,6 +251,27 @@ moduleFor('EventDispatcher#setup', class extends RenderingTest {
}
});

moduleFor('custom EventDispatcher subclass with #setup', class extends RenderingTest {
constructor() {
super();

let dispatcher = this.owner.lookup('event_dispatcher:main');
run(dispatcher, 'destroy');
this.owner.__container__.reset('event_dispatcher:main');
this.owner.unregister('event_dispatcher:main');
}

['@test canDispatchToEventManager is deprecated in EventDispatcher'](assert) {
let MyDispatcher = EventDispatcher.extend({
canDispatchToEventManager: null
});
this.owner.register('event_dispatcher:main', MyDispatcher);

expectDeprecation(/`canDispatchToEventManager` has been deprecated/);
this.owner.lookup('event_dispatcher:main');
}
});

if (EMBER_IMPROVED_INSTRUMENTATION) {
moduleFor('EventDispatcher - Instrumentation', class extends RenderingTest {
teardown() {
Expand Down
9 changes: 7 additions & 2 deletions packages/ember-views/lib/system/event_dispatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { deprecate } from 'ember-debug';
import { Object as EmberObject } from 'ember-runtime';
import jQuery from './jquery';
import ActionManager from './action_manager';
import { environment } from 'ember-environment';
import fallbackViewRegistry from '../compat/fallback-view-registry';

const ROOT_ELEMENT_CLASS = 'ember-application';
Expand Down Expand Up @@ -136,7 +135,13 @@ export default EmberObject.extend({

init() {
this._super();
assert('EventDispatcher should never be instantiated in fastboot mode. Please report this as an Ember bug.', environment.hasDOM);

assert('EventDispatcher should never be instantiated in fastboot mode. Please report this as an Ember bug.', (() => {
let owner = getOwner(this);
let environment = owner.lookup('-environment:main');

return environment.isInteractive;
})());

deprecate(
`\`canDispatchToEventManager\` has been deprecated in ${this}.`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ const TextNode = window.Text;
export default class AbstractRenderingTestCase extends AbstractTestCase {
constructor() {
super();
let bootOptions = this.getBootOptions();

let owner = this.owner = buildOwner({
ownerOptions: this.getOwnerOptions(),
bootOptions: this.getBootOptions(),
resolver: this.getResolver()
resolver: this.getResolver(),
bootOptions,
});

this.renderer = this.owner.lookup('renderer:-dom');
Expand All @@ -24,7 +26,9 @@ export default class AbstractRenderingTestCase extends AbstractTestCase {

owner.register('event_dispatcher:main', EventDispatcher);
owner.inject('event_dispatcher:main', '_viewRegistry', '-view-registry:main');
owner.lookup('event_dispatcher:main').setup(this.getCustomDispatcherEvents(), this.element);
if (!bootOptions || bootOptions.isInteractive !== false) {
owner.lookup('event_dispatcher:main').setup(this.getCustomDispatcherEvents(), this.element);
}
}

compile() {
Expand Down

0 comments on commit 3727e2e

Please sign in to comment.