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

cleanup usage of Ember.inject #6356

Merged
merged 1 commit into from
Jan 19, 2016
Merged

Conversation

acburdine
Copy link
Member

no issue

cleans up some const usage of Ember.inject and a couple of other Ember globals

@acburdine
Copy link
Member Author

@kevinansfield Not sure if this is needed/wanted at all, I just noticed this pattern in ember-ajax and thought it might clean up the code a little bit

@kevinansfield
Copy link
Contributor

👍 Looks like a nice improvement! A couple of things:

  1. It would be good to standardise how we call service(), either always specifying the name, e.g. notifications: service('notifications') or only specifying the name when needed, notifications: service(). I'm leaning towards only specifying it when needed, in which case the ghost-paths and slug-generator service injects can be cleaned up.
  2. computed is a bit of an odd one for the destructed assignment as it is both used directly computed() and has descendants computed.alias(), as such I think we should stick to this format everywhere so there's no confusion about when to use const { computed: {alias} } vs const { computed } = Ember; const { alias } = computed:
const {
    computed
} = Ember;

const {
    alias,
    filter
} = computed;

@acburdine
Copy link
Member Author

Sounds good! Will re-do this a bit later today.

@acburdine acburdine force-pushed the inject-usage branch 3 times, most recently from 411cf02 to dd8ce46 Compare January 19, 2016 13:02
kevinansfield added a commit that referenced this pull request Jan 19, 2016
@kevinansfield kevinansfield merged commit 580234c into TryGhost:master Jan 19, 2016
@acburdine acburdine deleted the inject-usage branch April 1, 2016 23:00
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.

2 participants