Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify and modularize app/router initialization #10256

Merged
merged 11 commits into from
Jan 22, 2015
Merged

Simplify and modularize app/router initialization #10256

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
585a935
Actually set test location to ‘none’
Jan 21, 2015
0320b5a
Default resolver matches CLI resolver `main`
Jan 21, 2015
788580f
Simplify and modularize app/router initialization
Jan 21, 2015
0941344
Store an ApplicationInstance on the Application
Jan 21, 2015
9c2b8d4
Extract ApplicationInstance to separate file
tomdale Jan 21, 2015
434e9cc
ApplicationInstance should create container
tomdale Jan 22, 2015
cf55da2
Introduce instance initializers
tomdale Jan 22, 2015
73d3ee0
Call super from router willDestroy
Jan 22, 2015
496dfe7
Add tests for arrays of `before` and `after`
Jan 22, 2015
a16adec
Add documentation to ApplicationInstance props
Jan 22, 2015
e929b74
Pass arguments to _super
Jan 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 90 additions & 113 deletions packages/ember-application/lib/system/application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,8 @@ var Application = Namespace.extend(DeferredMixin, {
customEvents: null,

init: function() {
this._super();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._super.apply(this, arguments);


// Start off the number of deferrals at 1. This will be
// decremented by the Application's own `initialize` method.
this._readinessDeferrals = 1;
Expand All @@ -261,42 +263,19 @@ var Application = Namespace.extend(DeferredMixin, {
this.$ = jQuery;
}

// Create subclass of Ember.Router for this Application instance.
// This is to ensure that someone reopening `App.Router` does not
// tamper with the default `Ember.Router`.
this.Router = Router.extend();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we comment this as for global mode only, something we can shed in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there, in general, a way for us to note code that likely can be removed in 2.0? /cc @rwjblue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just // 2.0TODO: ...

this.buildRegistry();
this.buildContainer();

this.Router = this.defaultRouter();
// TODO:(tomdale+wycats) Move to session creation phase
this.buildInstance();

this._super();
registerLibraries();
logLibraryVersions();

this.scheduleInitialize();

if (!librariesRegistered) {
librariesRegistered = true;

if (environment.hasDOM) {
Ember.libraries.registerCoreLibrary('jQuery', jQuery().jquery);
}
}

if (Ember.LOG_VERSION) {
// we only need to see this once per Application#init
Ember.LOG_VERSION = false;
var libs = Ember.libraries._registry;

var nameLengths = EnumerableUtils.map(libs, function(item) {
return get(item, 'name.length');
});

var maxNameLength = Math.max.apply(this, nameLengths);

Ember.debug('-------------------------------');
for (var i = 0, l = libs.length; i < l; i++) {
var lib = libs[i];
var spaces = new Array(maxNameLength - lib.name.length + 1).join(' ');
Ember.debug([lib.name, spaces, ' : ', lib.version].join(''));
}
Ember.debug('-------------------------------');
}
},

/**
Expand All @@ -316,46 +295,19 @@ var Application = Namespace.extend(DeferredMixin, {
Create a container for the current application's registry.

@private
@method buildContainer
@method buildInstance
@return {Ember.Container} the configured container
*/
buildContainer: function() {
buildInstance: function() {
var container = this.__container__ = this.__registry__.container();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maintain __container__ on the Application as well as the ApplicationInstance? Removing it from the Application might not be worth it until 2.0, since it is so frequently accessed despite being "private" (e.g. from the Ember Inspector). Perhaps we should flag it for 2.0 removal?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgeb - Ember.Application.create().__container__ is not removed here (AFAICT). Are you suggesting that ApplicationInstance.create().__container__ should be used instead of ApplicationInstance.create().container?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, @wycats and I discussed this a bit and felt that, as much as this fact might pain us, the __container__ API is de facto public and removing it would be very annoying to a great many people. Given that it is not particularly onerous to continue supporting it, at least until 2.0, we thought it best to preserve it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting that ApplicationInstance.create().__container__ should be used instead of ApplicationInstance.create().container?

@rwjblue I wasn't suggesting this, but I'd recommend doing so IF ApplicationInstance becomes public itself. Since that doesn't appear to be the case in this PR, I'd hold off until then.

Given that it is not particularly onerous to continue supporting it, at least until 2.0, we thought it best to preserve it.

@tomdale Those are my thoughts as well. Perhaps tag it with the 2.0TODO flag mentioned above?


return container;
},

/**
If the application has not opted out of routing and has not explicitly
defined a router, supply a default router for the application author
to configure.

This allows application developers to do:

```javascript
var App = Ember.Application.create();

App.Router.map(function() {
this.resource('posts');
var instance = this.__instance__ = ApplicationInstance.create({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should ApplicationInstance by instead managed by the container itself, that way router:main' andevent:dispatcher` can be injected and theoretically resolved, allowing us to continue to move away from explicit registration as a mechanism as specifying a default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say more what you mean? The ApplicationInstance is responsible for creating the container.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the container is created immediately before the application instance. The container is even passed to the application instance on create.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the bootstrap process should be to instantiate via the container.

customEvents: get(this, 'customEvents'),
rootElement: get(this, 'rootElement'),
container: container
});
```

@private
@method defaultRouter
@return {Ember.Router} the default router
*/

defaultRouter: function() {
if (this.Router === false) { return; }
var container = this.__container__;
var registry = this.__registry__;

if (this.Router) {
registry.unregister('router:main');
registry.register('router:main', this.Router);
}

return container.lookupFactory('router:main');
return instance;
},

/**
Expand Down Expand Up @@ -573,17 +525,6 @@ var Application = Namespace.extend(DeferredMixin, {
_initialize: function() {
if (this.isDestroyed) { return; }

// At this point, the App.Router must already be assigned
if (this.Router) {
var registry = this.__registry__;
var container = this.__container__;

registry.unregister('router:main');
registry.register('router:main', this.Router);

container.reset('router:main');
}

this.runInitializers();
runLoadHooks('application', this);

Expand Down Expand Up @@ -664,15 +605,14 @@ var Application = Namespace.extend(DeferredMixin, {
@method reset
**/
reset: function() {
var instance = this.__instance__;

this._readinessDeferrals = 1;

function handleReset() {
var router = this.__container__.lookup('router:main');
router.reset();

run(this.__container__, 'destroy');
run(instance, 'destroy');

this.buildContainer();
this.buildInstance();

run.schedule('actions', this, '_initialize');
}
Expand Down Expand Up @@ -716,7 +656,7 @@ var Application = Namespace.extend(DeferredMixin, {
*/
didBecomeReady: function() {
if (environment.hasDOM) {
this.setupEventDispatcher();
this.__instance__.setupEventDispatcher();
}

this.ready(); // user hook
Expand All @@ -731,23 +671,6 @@ var Application = Namespace.extend(DeferredMixin, {
this.resolve(this);
},

/**
Setup up the event dispatcher to receive events on the
application's `rootElement` with any registered
`customEvents`.

@private
@method setupEventDispatcher
*/
setupEventDispatcher: function() {
var customEvents = get(this, 'customEvents');
var rootElement = get(this, 'rootElement');
var dispatcher = this.__container__.lookup('event_dispatcher:main');

set(this, 'eventDispatcher', dispatcher);
dispatcher.setup(customEvents, rootElement);
},

/**
If the application has a router, use it to route to the current URL, and
trigger a new call to `route` whenever the URL changes.
Expand All @@ -757,18 +680,12 @@ var Application = Namespace.extend(DeferredMixin, {
@property router {Ember.Router}
*/
startRouting: function() {
var router = this.__container__.lookup('router:main');
if (!router) { return; }

var moduleBasedResolver = this.Resolver && this.Resolver.moduleBasedResolver;

router.startRouting(moduleBasedResolver);
var isModuleBasedResolver = this.Resolver && this.Resolver.moduleBasedResolver;
this.__instance__.startRouting(isModuleBasedResolver);
},

handleURL: function(url) {
var router = this.__container__.lookup('router:main');

router.handleURL(url);
return this.__instance__.handleURL(url);
},

/**
Expand All @@ -795,12 +712,10 @@ var Application = Namespace.extend(DeferredMixin, {
*/
Resolver: null,

// This method must be moved to the application instance object
willDestroy: function() {
Ember.BOOTED = false;
// Ensure deactivation of routes before objects are destroyed
this.__container__.lookup('router:main').reset();

this.__container__.destroy();
this.__instance__.destroy();
},

initializer: function(options) {
Expand Down Expand Up @@ -1016,7 +931,6 @@ Application.reopenClass({
registry.register('route:basic', Route, { instantiate: false });
registry.register('event_dispatcher:main', EventDispatcher);

registry.register('router:main', Router);
registry.injection('router:main', 'namespace', 'application:main');

registry.register('location:auto', AutoLocation);
Expand Down Expand Up @@ -1097,4 +1011,67 @@ function resolverFor(namespace) {
return resolve;
}

function registerLibraries() {
if (!librariesRegistered) {
librariesRegistered = true;

if (environment.hasDOM) {
Ember.libraries.registerCoreLibrary('jQuery', jQuery().jquery);
}
}
}

function logLibraryVersions() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are refactoring, it might be better to declare these as

var logLibraryVersions;

// later
Ember.runInDebug(function(){
  logLibraryVersions = function(){

  };
});


// init Function
Ember.runInDebug(function(){
  logLibraryVersions();
});

and use Ember.runInDebug. Right now, emberjs-build is configured to strip out Ember.debug statements. So, if this code ultimately becomes a no op (Ember.debug is the only output here, no other side effects), it'd be nice if it could be stripped out at build time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetanley - I agree in principle, but having Ember.debug nested inside of Ember.runInDebug would throw errors (because defeatureify doesn't like nested stripped blocks). Some more finagling would be required to deal with that (still totally doable though).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy to do whatever here, we just extracted these as-is to make the init method easier to read.

if (Ember.LOG_VERSION) {
// we only need to see this once per Application#init
Ember.LOG_VERSION = false;
var libs = Ember.libraries._registry;

var nameLengths = EnumerableUtils.map(libs, function(item) {
return get(item, 'name.length');
});

var maxNameLength = Math.max.apply(this, nameLengths);

Ember.debug('-------------------------------');
for (var i = 0, l = libs.length; i < l; i++) {
var lib = libs[i];
var spaces = new Array(maxNameLength - lib.name.length + 1).join(' ');
Ember.debug([lib.name, spaces, ' : ', lib.version].join(''));
}
Ember.debug('-------------------------------');
}
}

var ApplicationInstance = Ember.Object.extend({
container: null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why these 3 properties exist? Or are they for future documentation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? They are used in the setupEventDispatcher implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are provided to the constructor, why do we need to specify them on the prototype?

customEvents: null,
rootElement: null,

startRouting: function(isModuleBasedResolver) {
var router = this.container.lookup('router:main');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.container.has('router:main') as a guard to this function seems more appropriate, no reason to startRouting if their is no router.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be possible to get to startRouting() without a router, I'd rather just remove the guard. That said, why is calling has() followed by lookup() preferable to checking for falsy values of lookup()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the intent is only to guard against the calling startRouting has makes more sense?

if (container.has('router:main')) {
  this.startRouting();
}

if (!router) { return; }

router.startRouting(isModuleBasedResolver);
},

handleURL: function(url) {
var router = this.container.lookup('router:main');

return router.handleURL(url);
},

setupEventDispatcher: function() {
var dispatcher = this.container.lookup('event_dispatcher:main');

dispatcher.setup(this.customEvents, this.rootElement);

return dispatcher;
},

willDestroy: function() {
run(this.container, 'destroy');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

join or is nested run-loop the intent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just preserving semantics you introduced in 9c83112. We specifically were trying to hew to existing behavior as much as possible without doing a more in-depth refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomdale and that used join. The above will nest a run loop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, if memory serves we did want a nested loop here. SG

}
});

export default Application;
15 changes: 11 additions & 4 deletions packages/ember-application/lib/system/resolver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,16 +169,15 @@ export default EmberObject.extend({
resolved = this[resolveMethodName](parsedName);
}

if (!resolved) {
resolved = this.resolveOther(parsedName);
}
resolved = resolved || this.resolveOther(parsedName);

if (parsedName.root && parsedName.root.LOG_RESOLVER) {
this._logLookup(resolved, parsedName);
}

return resolved;
},

/**
Convert the string name of the form 'type:name' to
a Javascript object with the parsed aspects of the name
Expand Down Expand Up @@ -214,13 +213,15 @@ export default EmberObject.extend({
' namespace, but the namespace could not be found', root);
}

var resolveMethodName = fullNameWithoutType === 'main' ? 'Main' : classify(type);

return {
fullName: fullName,
type: type,
fullNameWithoutType: fullNameWithoutType,
name: name,
root: root,
resolveMethodName: 'resolve' + classify(type)
resolveMethodName: 'resolve' + resolveMethodName
};
},

Expand Down Expand Up @@ -253,6 +254,7 @@ export default EmberObject.extend({
makeToString: function(factory, fullName) {
return factory.toString();
},

/**
Given a parseName object (output from `parseName`), apply
the conventions expected by `Ember.Router`
Expand Down Expand Up @@ -368,6 +370,11 @@ export default EmberObject.extend({
if (factory) { return factory; }
},

resolveMain: function(parsedName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add doc to this new method and annotate it public/private

var className = classify(parsedName.type);
return get(parsedName.root, className);
},

/**
@method _logLookup
@param {Boolean} found
Expand Down
2 changes: 1 addition & 1 deletion packages/ember-application/tests/system/application_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ test("can resolve custom router", function() {
var CustomRouter = Router.extend();

var CustomResolver = DefaultResolver.extend({
resolveOther: function(parsedName) {
resolveMain: function(parsedName) {
if (parsedName.type === "router") {
return CustomRouter;
} else {
Expand Down
6 changes: 6 additions & 0 deletions packages/ember-application/tests/system/dependency_injection/default_resolver_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ test("the default resolver resolves models on the namespace", function() {
detectEqual(application.Post, locator.lookupFactory('model:post'), "looks up Post model on application");
});

test("the default resolver resolves *:main on the namespace", function() {
application.FooBar = EmberObject.extend({});

detectEqual(application.FooBar, locator.lookupFactory('foo-bar:main'), "looks up FooBar type without name on application");
});

test("the default resolver resolves helpers", function() {
expect(2);

Expand Down
4 changes: 4 additions & 0 deletions packages/ember-routing/lib/system/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,10 @@ var EmberRouter = EmberObject.extend(Evented, {
}
},

willDestroy: function() {
this.reset();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._super.apply(this, arguments);

},

_lookupActiveView: function(templateName) {
var active = this._activeViews[templateName];
return active && active[0];
Expand Down
2 changes: 1 addition & 1 deletion packages/ember/tests/application_lifecycle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ QUnit.module("Application Lifecycle", {
rootElement: '#qunit-fixture'
});

App.Router.extend({
App.Router = App.Router.extend({
location: 'none'
});

Expand Down