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

Run initializers and instance-initializers inside engines #96

Merged
merged 3 commits into from May 3, 2016

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented May 3, 2016

This enables initializers and instance-initializers inside engines.

The only change necessary to get instance-initializers running is altering the canonical addon/engine.js to invoke ember-load-initializers (which is analogous to what happens in app/app.js).

Getting the initializers running required an extention to the Engine class. Applications run them when they're told to boot, but no equivalent step happens for child engines, so instead we run them the first time we build an engine instance.

This enables initializers and instance-initializers inside engines.

The only change necessary to get instance-initializers running is altering the canonical addon/engine.js to invoke ember-load-initializers (which is analogous to what happens in app/app.js).

Getting the initializers running required an extention to the Engine class. Applications run them when they're told to boot, but no equivalent step happens for child engines, so instead we run them the first time we build an engine instance.

export default Engine.extend({
modulePrefix: 'ember-blog',
let Eng;
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 tweak this to something like the following:

const modulePrefix = 'ember-blog';
const Eng = Engine.extend({
  modulePrefix,
  Resolver
});

loadInitializers(Eng, modulePrefix);

Reading it top to bottom like this seems easier to my brain 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I was just blindly copying the format of the app/app.js blueprint. This is much better.

@rwjblue
Copy link
Member

rwjblue commented May 3, 2016

Applications run them when they're told to boot, but no equivalent step happens for child engines

I do not think this is quite correct. We should be running initializers during init of the Engine instance (this is roughly what happens with Ember.Application also).


Can you another test that ensures all initializers are ran before instance initializers? I am also concerned about an initializer calling deferReadiness on the engine (which IMO we should not support but should provide a helpful error).

Can you also update the blueprints (here and here) to follow the recommendations in the README?

@ef4
Copy link
Contributor Author

ef4 commented May 3, 2016

I do not think this is quite correct. We should be running initializers during init of the Engine instance (this is roughly what happens with Ember.Application also).

Application doesn't run them during init, it waits until domReady() or boot().

@rwjblue
Copy link
Member

rwjblue commented May 3, 2016

Application doesn't run them during init, it waits until domReady() or boot().

You are correct, but they are called as a byproduct of calling Ember.Application.create (there is a bit async there due to the domReady checks). Since we do not intend to support async like that, we should fire the initializers just after init is finished (perhaps on('init', function() { this.runInitializers(); })).

@rwjblue
Copy link
Member

rwjblue commented May 3, 2016

Anyways, I don't think that solution really ends up being different than this. And arguably this is clearer...

@dgeb
Copy link
Member

dgeb commented May 3, 2016

I am fine with this approach to Engine init for now, with the understanding that we will refine it when we introduce async loading of engines.

@rwjblue rwjblue merged commit b792db4 into ember-engines:master May 3, 2016
@rwjblue
Copy link
Member

rwjblue commented May 3, 2016

Thanks @ef4!

@ef4
Copy link
Contributor Author

ef4 commented May 3, 2016

Oh, one small nitpick: this change technically makes every engine need to put ember-load-initializers into dependencies. Which doesn't even work, due to the dreaded nested included issue.

But in practice every ember-cli app already has ember-load-initializers, so it seems OK.

@rwjblue
Copy link
Member

rwjblue commented May 3, 2016

Yes, and they can easily not include it if they don't need/want initializers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants