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.

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

Simplify and modularize app/router initialization #10256

Merged
merged 11 commits into from Jan 22, 2015

Conversation

@tomdale
Copy link
Member

commented Jan 21, 2015

Ember.Application's initialization has evolved organically a number of times to support the transition from no router, to a globals-based router, and now the Ember CLI/ES6 modules world. Over time, this initialization code has accumulated some cruft that makes it difficult to reason about its behavior.

This carefully constructed series of targeted refactors preserves the previous semantics as closely as possible (with one exception that we may want to roll back depending on real-world impact; see below). This refactoring is in anticipation of our work to support multiple sessions of a single Ember.Application instance for the FastBoot effort.


The one exception to semantics-preservation is a small tweak to the globals resolver to bring it more in line with the Ember CLI resolver.

In the Ember CLI resolver, full paths named something:main would resolve to appname/something.js–the main suffix is special-cased to become a top-level lookup. In order to achieve something similar in the globals resolver, there are hardcoded registrations (registry.register('foo:main', Foo)) for router and store.

We changed the globals strategy to be more like the Ember CLI strategy: any something:main lookup in globals mode gets resolved to App.Something.

This is a slight change in semantics because the process of going from a full name to a factory first tries to go through the resolver before using any explicitly registered full names.

In order for this to become a problem:

  • An application would need to explicitly register a foo:main in an initializer with a hardcoded value that did not live at App.Foo (e.g. registering web-socket:main with a hardcoded class other than App.WebSocket).
  • They would also need to have an App.WebSocket model in their application

Note that this problem does not exist for App.Router or App.Store, since they have always been special-cased by Ember and Ember Data respectively.

If necessary, we could restrict the new behavior to router:main, but if we can get away with this minor semantics change, we think this would more closely align with how people already expect this to work, and how it does work in Ember CLI.

Tom Dale and Yehuda Katz added some commits Jan 21, 2015

Default resolver matches CLI resolver `main`
The CLI resolver always treats type keys with the name of `main` as
top-level objects. For example, `router:main` resolves to `app/router.js`
instead of `app/routers/main.js`.

This change updates the default resolver to always look for
`foo-bar:main` as `App.FooBar`. This is in anticipation of application
init/boot cleanup that we are doing as part of the FastBoot effort.
}
}

function logLibraryVersions() {

This comment has been minimized.

Copy link
@fivetanley

fivetanley Jan 21, 2015

Member

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.

This comment has been minimized.

Copy link
@rwjblue

rwjblue Jan 21, 2015

Member

@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).

This comment has been minimized.

Copy link
@tomdale

tomdale Jan 21, 2015

Author Member

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

Store an ApplicationInstance on the Application
Instead of storing a `__container__` on the Application, this commit
stores an ApplicationInstance, which manages the per-instance lifecycle.

The next step will be allowing multiple `ApplicationInstance`s to exist
at once, enabling a single app to serve multiple FastBoot requests at a
time.

This is a conceptual improvement that brings application “reset” in line
with the destruction infrastructure.

As part of this commit, we eliminated an ad-hoc call to router.reset(),
allowing that logic to happen as a natural consequence of destruction
of the `ApplicationInstance`.

In general, the goal of this commit is to move all responsibility for
application state into an object that manages it.
@@ -253,6 +253,8 @@ var Application = Namespace.extend(DeferredMixin, {
customEvents: null,

init: function() {
this._super();

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

this._super.apply(this, arguments);

}

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

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

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

This comment has been minimized.

Copy link
@tomdale

tomdale Jan 21, 2015

Author Member

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

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

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

rootElement: null,

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

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

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

This comment has been minimized.

Copy link
@tomdale

tomdale Jan 21, 2015

Author Member

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()?

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

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

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

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

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

join or is nested run-loop the intent.

This comment has been minimized.

Copy link
@tomdale

tomdale Jan 21, 2015

Author Member

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.

This comment has been minimized.

Copy link
@krisselden

krisselden Jan 21, 2015

Member

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

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

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

@@ -297,6 +297,10 @@ var EmberRouter = EmberObject.extend(Evented, {
}
},

willDestroy: function() {
this.reset();

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

this._super.apply(this, arguments);

// 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();

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

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

This comment has been minimized.

Copy link
@tomdale

tomdale Jan 21, 2015

Author Member

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

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

maybe just // 2.0TODO: ...

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

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

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.

This comment has been minimized.

Copy link
@tomdale

tomdale Jan 21, 2015

Author Member

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

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

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

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 21, 2015

Member

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

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jan 21, 2015

looks good, interested in seeing how this refactoring plan progresses.

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

This comment has been minimized.

Copy link
@dgeb

dgeb Jan 21, 2015

Member

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?

This comment has been minimized.

Copy link
@rwjblue

rwjblue Jan 21, 2015

Member

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

This comment has been minimized.

Copy link
@tomdale

tomdale Jan 21, 2015

Author Member

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.

This comment has been minimized.

Copy link
@dgeb

dgeb Jan 21, 2015

Member

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?

@dgeb

This comment has been minimized.

Copy link
Member

commented Jan 21, 2015

Looks like a good incremental refactor 👍

I still have some questions about our next steps (re: per-instance registries and the upcoming changes to initializers) but those can be dealt with down the line.

@wagenet wagenet added the Cleanup label Jan 21, 2015

ApplicationInstance should create container
Based on feedback from @stefanpenner, we moved responsibility for
creating the default container from the `Application` to the
`ApplicationInstance`. Instead, the instance points back at the
application’s registry, using it as the basis for a new container.

This commit also adds initial documentation to the application instance.
@stefanpenner

This comment has been minimized.

this._super.apply(this, arguments);

@stefanpenner

This comment has been minimized.

Copy link
Member

commented on 434e9cc Jan 22, 2015

although maybe i wasn't clear, i believe my advice was for the ApplicationInstance to be initiated via the container, not to do the actually container instantiation.

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2015

We also introduced a deprecation guide for the recent registry/container split, and moved that deprecation behind a feature flag. Once this PR is merged, we should make sure that emberjs/website#1951 is merged along with it so that the deprecation URL works correctly.

Introduce instance initializers
This commit introduces a new (feature flagged) API for adding
instance initializers.

Instance initializers differ from normal initializers in that they are
passed the app instance rather than a registry, and therefore can access
instances from the container in a safe way.

This design not only allows us to avoid expensive app setup for each
FastBoot request, it also minimizes the amount of work required
between acceptance test runs (once the testing infrastructure
is updated to take advantage of it).

This commit also removes a previously introduced deprecation that was
not behind a feature flag. That deprecation (when emitted with the
feature flag enabled) now points to a comprehensive deprecation guide.

@tomdale tomdale force-pushed the app-sessions branch to cf55da2 Jan 22, 2015

@rwjblue

This comment has been minimized.

Copy link
Member

commented Jan 22, 2015

@tomdale - I realize that you are not changing the fundamentals here, but I'd love to see a test with before and after as an array (just to confirm we don't regress there) for both initializer and instanceInitializer.

Tom Dale and Yehuda Katz added some commits Jan 22, 2015

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2015

@rwjblue As requested: 496dfe7

@@ -368,6 +370,11 @@ export default EmberObject.extend({
if (factory) { return factory; }
},

resolveMain: function(parsedName) {

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 22, 2015

Member

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

import run from "ember-metal/run_loop";
import Application from "ember-application/system/application";
import ApplicationInstance from "ember-application/system/application-instance";
import { indexOf } from "ember-metal/array";

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 22, 2015

Member

enumerableUtils is actually the right place to grab this

rootElement: null,

init: function() {
this._super();

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 22, 2015

Member

this._super.apply(this, arguments);

Tom Dale and Yehuda Katz added some commits Jan 22, 2015

rwjblue added a commit that referenced this pull request Jan 22, 2015

Merge pull request #10256 from emberjs/app-sessions
Simplify and modularize app/router initialization

@rwjblue rwjblue merged commit 6baf493 into master Jan 22, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@rwjblue rwjblue deleted the app-sessions branch Jan 22, 2015

@OrKoN

This comment has been minimized.

Copy link

commented May 7, 2015

Will instance initializers always run after all normal initializers?

mylen added a commit to mylen/mantrailling that referenced this pull request Jun 5, 2015

@fpauser

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2015

How to migrate this initializer, which needs to access a service, waits for a resolving promise and dynamically sets up routes?

import Router from '../router';

export function initialize(registry, application) {
  var session = registry.lookup('service:session');

  application.deferReadiness();

  session.load()
    .then(function(config) {
      if (config.fancyRoute) {
        Router.map(function() {
          this.route('fancyRoute');
        });
      }
      application.advanceReadiness();
    })
    .catch(function(e) {
      Ember.Logger.error('[session-initializer]', e);
    });
}

export default {
  name: 'session',
  initialize: initialize
};
@OrKoN

This comment has been minimized.

Copy link

commented Jun 11, 2015

I support the question by @fpauser Could you suggest how to migrate such cases @tomdale @rwjblue ? Thanks.

I tried to use deferReadiness in the instance initializer http://emberjs.jsbin.com/xutebutoma/1/edit?html,css,js,console,output But it seems it's not possible or I am doing it wrong.

In my case, we have custom dependency management solution for initializers https://github.com/n-fuse/ember-initializer-dm/blob/master/main.js. It assumes that each of the initializers returns a Promise and defines its dependencies via needs array. Some of initializers also generate Router's map based on the server configuration. We're looking for a way to migrate this part while maintaining the possibility to have dynamic routes.

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2015

@OrKoN Yes, instance initializers should always run after app initializers. If this is not happening for you please file an issue.

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2015

@fpauser Since your initializer is changing state based on a service, and services can change between app instances, you should move that code to an instance initializer. (Instantiating services before the app is booted could definitely lead to some undefined/strange behavior, for what it's worth.)

I'm not sure if we support mutating the route map after the app instance has booted, but we should expose APIs for doing so if not.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Jun 11, 2015

I'm not sure if we support mutating the route map after the app instance has booted, but we should expose APIs for doing so if not.

There is an open issue for this: #10979. It worked in 1.10 but not 1.11+ (possibly due to changes here)...

@OrKoN

This comment has been minimized.

Copy link

commented Jun 12, 2015

@rwjblue @tomdale Thanks for your comments. I've got one more question though: how to defer the app readiness in an instance initializer? In my app, all routes are created in initializers so it needs to be done before the app starts and I don't see how it's possible w/o the deferReadiness method.

@OrKoN

This comment has been minimized.

Copy link

commented Jun 12, 2015

I have created a jsbin http://emberjs.jsbin.com/zixivojiyi/1/edit?js,console,output to demonstrate the use case. It's not working because I don't know how to make the instance initializer wait for the promise outcome.

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2015

@OrKoN It's not possible to defer app readiness in an instance initializer, since by definition instance initializers are only run after the app has finished booting.

Sidebar on the semantics of application booting: "App readiness" (as in, deferReadiness() and advanceReadiness()) refers to whether all of the code for the application has loaded. Once all of the code has loaded, a new instance is created, which is your application.

To restate, the lifecycle of an Ember application running in the browser is:

  1. Ember loads.
  2. You create an Ember.Application instance global (e.g. App).
  3. At this point, none of your classes have been loaded yet.
  4. As your JavaScript file is evaluated, you register classes on the application (e.g. App.MyController = Ember.Controller.extend(…);)
  5. Ember waits for DOM ready to ensure that all of your JavaScript included via <script> tags has loaded.
  6. Initializers are run.
  7. If you need to lazily load code or wait for additional setup, you can call deferReadiness().
  8. Once everything is loaded, you can call advanceReadiness().
  9. At this point, we say that the Application is ready; in other words, we have told Ember that all of the classes (components, routes, controllers, etc.) that make up the app are loaded.
  10. A new instance of the application is created, and instance initializers are run.
  11. Routing starts and the UI is rendered to the screen.

If you want to delay showing the UI because there is some runtime setup you need to do (for example, you want to open a WebSocket before the app starts running), the correct solution is to use the beforeModel/model/afterModel hooks in the ApplicationRoute. All of these hooks allow you to return a promise that will prevent child routes from being evaluated until they resolve.

Using deferReadiness() in an initializer is an unfortunate hack that many people have come to rely on. I call it a hack because, unlike the model promise chain in the router, it breaks things like error and loading substates. By blocking rendering in initializers, IMO you are creating a worse experience for users because they will not see a loading or error substate if the promise is slow or rejects, and most of the code I've seen doesn't have any error handling code at all. This leads to apps that can break with just a blank white screen and no indication to the user that something bad has happened.

@berkus

This comment has been minimized.

Copy link

commented Jul 4, 2015

@tomdale this is awesome description and I wish I could find it somewhere in the official documentation!

@locks locks referenced this pull request Jul 5, 2015
0 of 1 task complete
@albertpak

This comment has been minimized.

Copy link

commented Jul 9, 2015

^^^ 👍 would be great to add it to ember guides

@greyhwndz

This comment has been minimized.

Copy link

commented Aug 31, 2015

@tomdale: :+5: Thanks for that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.