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

Execute initializers in their respective context #10179

Merged
merged 1 commit into from
Jan 13, 2015

Conversation

gf3
Copy link
Contributor

@gf3 gf3 commented Jan 9, 2015

Currently initializers are executed in the global context; this small but useful change will execute the initialize function in the context of its object. Below is an example of where something like this would come in handy. In the example sometimes it's useful (especially with testing) to be able to reset app-state or to switch out various app states at different times.

import Ember from 'ember';

export default {
  name: 'app-state',

  state() {
    return Ember.Object.create({
      components:  Ember.Object.create(),
      controllers: Ember.Object.create(),
      routes:      Ember.Object.create(),
      views:       Ember.Object.create()
    });
  },

  initialize(container, app) {
    app.register('state:main', this.state(), { instantiate: false });

    app.inject('component',  'app-state', 'state:main');
    app.inject('controller', 'app-state', 'state:main');
    app.inject('route',      'app-state', 'state:main');
    app.inject('view',       'app-state', 'state:main');
  }
};

@trek
Copy link
Member

trek commented Jan 9, 2015

👍, although we should probably discuss whether this needs to be flagged and/or is a breaking change as written.

@gf3
Copy link
Contributor Author

gf3 commented Jan 9, 2015

@trek Ahh yeah, I wasn't sure about that. Let me know if you think it needs to be behind a flag and I'll update.

@trek
Copy link
Member

trek commented Jan 9, 2015

Off the top of my head I can't think of a sensible reason to rely on global execution, but that doesn't mean it's not a possibly widely used "feature" that we can't break for 2.0. I'll add it to agenda for the next core team meeting.

@gf3
Copy link
Contributor Author

gf3 commented Jan 9, 2015

@trek Splendid, thank you!

@trek trek self-assigned this Jan 9, 2015
@tomdale
Copy link
Member

tomdale commented Jan 9, 2015

This seems like an improvement to me, but we should definitely get it feature flagged.

@@ -693,9 +693,15 @@ var Application = Namespace.extend(DeferredMixin, {
var namespace = this;
var initializer;

var makeGraphValue = function(initializer) {
return function() {
return initializer.initialize.apply(initializer, arguments);
Copy link
Member

Choose a reason for hiding this comment

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

The arguments are known, why not just specify them (as opposed to calling apply)?

Choose a reason for hiding this comment

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

@rwjblue It may may helpful when adding/changing the arguments of initialize function in future (not sure).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manoharank @rwjblue Indeed, future-proofing was my intent here.

Copy link
Member

Choose a reason for hiding this comment

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

Using apply is slower than just calling the function and if we refactor the arguments then this may need to be updated anyways (along with many other things). tldr; I would prefer to just invoke the function with the params that we know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue We can't simply call the function, we need to use either call or apply. I can update the PR to use call with named arguments if you would prefer?

Copy link
Member

Choose a reason for hiding this comment

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

@gf3 - Did you test it as I suggested? Calling a function from an object should set this inside the function to the object that it was called from.

return initializer.initialize(container, application);

Sample JSBin: http://jsbin.com/wefas/1/edit?html,js,output

@gf3
Copy link
Contributor Author

gf3 commented Jan 13, 2015

@trek @tomdale Should I add the feature flag?

@trek
Copy link
Member

trek commented Jan 13, 2015

@gf3 yes please.

@gf3 gf3 force-pushed the initializer-context branch 2 times, most recently from a9e1f2e to bbb7659 Compare January 13, 2015 17:15
@gf3
Copy link
Contributor Author

gf3 commented Jan 13, 2015

@trek @tomdale Updated—should be good to go!

@@ -14,7 +14,8 @@
"new-computed-syntax": null,
"ember-testing-checkbox-helpers": null,
"ember-metal-stream": null,
"ember-htmlbars-each-with-index": true
"ember-htmlbars-each-with-index": true,
"ember-initializer-context": null
Copy link
Member

Choose a reason for hiding this comment

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

We typically like to use the package name as the prefix. Can you update to ember-application-initializer-context?

rwjblue added a commit that referenced this pull request Jan 13, 2015
Execute initializers in their respective context
@rwjblue rwjblue merged commit 1d48dad into emberjs:master Jan 13, 2015
@rwjblue
Copy link
Member

rwjblue commented Jan 13, 2015

@gf3 - Thanks!

@gf3 gf3 deleted the initializer-context branch January 13, 2015 22:21
nathanhammond added a commit to nathanhammond/ember-cli that referenced this pull request Sep 13, 2015
The initializer API has changed as of 2.X, the blueprints need to be updated to address those changes. See also: emberjs/ember.js#10179
nathanhammond added a commit to nathanhammond/ember-cli that referenced this pull request Sep 13, 2015
The initializer API has changed as of 2.X, the blueprints need to be updated to address those changes. See also: emberjs/ember.js#10179
nathanhammond added a commit to nathanhammond/ember-cli that referenced this pull request Sep 13, 2015
nathanhammond added a commit to nathanhammond/ember-cli that referenced this pull request Oct 2, 2015
nathanhammond added a commit to nathanhammond/ember-cli that referenced this pull request Oct 2, 2015
homu added a commit to ember-cli/ember-cli that referenced this pull request Oct 2, 2015
[Safe for 2.1] Update initializer blueprint.

Fixes the test blueprint to invoke the initializer in a way such that `this` is set. See: emberjs/ember.js#10179
homu added a commit to ember-cli/ember-cli that referenced this pull request Oct 2, 2015
[Safe for 2.1] Update initializer blueprint.

Fixes the test blueprint to invoke the initializer in a way such that `this` is set. See: emberjs/ember.js#10179
homu added a commit to ember-cli/ember-cli that referenced this pull request Oct 7, 2015
[Safe for 2.1] Update initializer blueprint.

Fixes the test blueprint to invoke the initializer in a way such that `this` is set. See: emberjs/ember.js#10179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants