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 3 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
118 changes: 42 additions & 76 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();

// TODO:(tomdale+wycats) Move to session creation phase
this.buildContainer();

this.Router = this.defaultRouter();

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 Down Expand Up @@ -325,39 +304,6 @@ var Application = Namespace.extend(DeferredMixin, {
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');
});
```

@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');
},

/**
Automatically initialize the application once the DOM has
become ready.
Expand Down Expand Up @@ -573,17 +519,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 @@ -1016,7 +951,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 +1031,36 @@ 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('-------------------------------');
}
}

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
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