Skip to content
This repository has been archived by the owner on May 26, 2019. It is now read-only.

Introduce "Application Concerns" section. #848

Merged
merged 1 commit into from
Oct 2, 2015

Conversation

dgeb
Copy link
Member

@dgeb dgeb commented Oct 1, 2015

Replaces the "Services and Initializers" section and expands its
content to include pages explaining Applications and Instances, as
well as Dependency Injection.

All the examples and text have been reviewed and updated for
clarity and compatibility with Ember 2.1.

One caveat is that the Initializers page references the
instance-initializer generator, which is still a WIP at the
moment. I'll try to get that into CLI ASAP.

Closes #827, #789, and #771.

the key `logger:main`:

```app/initializers/logger.js
export function initialize(application) {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Exporting a bare function like this is OK? Doesn't this need the Ember.Application.initializer( call?

(applies to several code snippets through this content)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, this is only a partial initializer implementation. The part that's left off would be:


export default {
  name: 'logger',
  initialize: initialize
};

I should just add this for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

... and to all the other initializer examples on this page.

@mixonic
Copy link
Sponsor Member

mixonic commented Oct 1, 2015

👏

@dgeb dgeb force-pushed the container-registry-reform branch 2 times, most recently from 4cc319f to 4308b37 Compare October 1, 2015 19:08
@dgeb
Copy link
Member Author

dgeb commented Oct 1, 2015

Thanks for the review @mixonic. Just pushed a revision.

Replaces the "Services and Initializers" section and expands its
content to include pages explaining Applications and Instances, as
well as Dependency Injection.

All the examples and text have been reviewed and updated for
clarity and compatibility with Ember 2.1.

One caveat is that the Initializers page references the
`instance-initializer` generator, which is still a WIP at the
moment. I'll try to get that into CLI ASAP.
@dgeb dgeb force-pushed the container-registry-reform branch from 4308b37 to 878d5b4 Compare October 1, 2015 21:03
@michaelrkn
Copy link
Contributor

This looks really awesome! I just have two thoughts about it:

  1. The Guides are typically structured from most common/useful features to know about, to more obscure/abstract ideas. The current order makes sense from the perspective of how an application works, but presents the most commonly-used feature (services) last. I'd suggest re-ordering it to:
  • Services
  • Initializers
  • Applications and Instances
  • Dependency Injection
  • The Container
  1. I'd also suggest we move the Run Loop guide into here. Currently it's in Components, which is really not great. It doesn't totally fit in with the rest of the content here, but it's much more closely aligned.

Thanks for the great work!

@dgeb
Copy link
Member Author

dgeb commented Oct 2, 2015

I agree that the run loop section would be a better fit here.

I think that we need to lead with the "applications and instances" section because the distinction is a bit confusing without an explanation, and the distinction is referenced throughout the initializers and DI sections.

The container section has been absorbed by the DI section and no longer exists on its own.

michaelrkn added a commit that referenced this pull request Oct 2, 2015
Introduce "Application Concerns" section.
@michaelrkn michaelrkn merged commit cbf67cf into emberjs:master Oct 2, 2015
@michaelrkn
Copy link
Contributor

Thanks for all your work on this, @dgeb!

@dgeb
Copy link
Member Author

dgeb commented Oct 2, 2015

@michaelrkn glad to help!

Let's revisit your ordering / organization concerns with separate PRs. I strongly agree about the run loop section being a better fit here than under components. Also, maybe the sub-section order could be tweaked a bit.

Still waiting to resolve the instance-initializer generator issue over in ember-cli/ember-cli#4218 Hope we can get that merged before the 2.1 guides go live - otherwise, we should make a note in the new instance initializer section.

@michaelrkn
Copy link
Contributor

Sounds good!!

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

Successfully merging this pull request may close these issues.

3 participants