Skip to content
This repository has been archived by the owner on Apr 18, 2023. It is now read-only.

GH-403: Add no-op ember-i18n initializer #404

Merged
merged 3 commits into from
Sep 13, 2016

Conversation

swquinn
Copy link
Contributor

@swquinn swquinn commented Sep 13, 2016

The removal of the ember-i18n initializer had unseen consequences for
applications that were injecting the i18n service into other areas of
the application, as recommended by documentation in the project's wiki
(https://github.com/jamesarosen/ember-i18n/wiki/Doc:-i18n-Service).

As per the discussion in GH-31 this adds a no-op initializer back into
the code base to prevent breaks for applications injecting the i18n
service as per the wiki.

The removal of the ember-i18n initializer had unseen consequences for
applications that were injecting the i18n service into other areas of
the application, as recommended by documentation in the project's wiki
(https://github.com/jamesarosen/ember-i18n/wiki/Doc:-i18n-Service).

As per the discussion in jamesarosenGH-31 this adds a no-op initializer back into
the code base to prevent breaks for applications injecting the i18n
service as per the wiki.
@jamesarosen
Copy link
Owner

Either this should be an instance-initializer, or we also need an instance-initializer.

@swquinn
Copy link
Contributor Author

swquinn commented Sep 13, 2016

I wasn't sure whether it should be an initializer and an instance-initializer or just the instance-initializer. I went with just the initializer at first because it seemed like it made the most sense to me.

Although, thinking about it now, I guess having both probably makes the most sense, in part because both existed previously. There isn't any sort of issue with initializers and instance-initializers sharing the same name, is there?

I'll change the PR to adopt the pattern used by the initializers previously, e.g.

// addon/initializers/ember-i18n
import instanceInitializer from "../instance-initializers/ember-i18n";

export default {
  name: instanceInitializer.name,
  initialize: function() {
    // No-op
  }
};

// addon/instance-initializers/ember-i18n
export default {
  name: 'ember-i18n',
  initialize() {
    // No-op.
  }
};

Does the above make sense?

@rwjblue
Copy link
Contributor

rwjblue commented Sep 13, 2016

Although, thinking about it now, I guess having both probably makes the most sense, in part because both existed previously.

Exactly. The idea here is to ensure that folks that setup before / after in their own initializers do not get errors because the internal ones do not exist.

I'll change the PR to adopt the pattern used by the initializers previously, e.g.

Perfect, thank you!

@jamesarosen
Copy link
Owner

There isn't any sort of issue with initializers and instance-initializers sharing the same name, is there?

As far as I can tell, they're completely separate graphs.

Does the above make sense?

It does! (The import to get the name seems like over-DRY-ing, but I'll merge whatever solves the problem 😃 )

@swquinn
Copy link
Contributor Author

swquinn commented Sep 13, 2016

Thanks for the feedback, guys! I added a few comments in (tried to follow the style I've seen elsewhere in the project for consistency). Let me know if there's anything else this PR needs before it can be merged!

As an aside, great work on this addon, I've worked with other i18n libraries that are really a pain to work with and this one has been a real pleasure to set up and use. I'm happy I could contribute back to it, even if its just something small like this.

@swquinn swquinn changed the title GH-31: Add no-op ember-i18n initializer GH-403: Add no-op ember-i18n initializer Sep 13, 2016
@swquinn
Copy link
Contributor Author

swquinn commented Sep 13, 2016

Also... sorry about the bad naming/tagging of the PR/first commit, I realized now that this was not a PR for GH-31 but rather GH-403. I think I must have seen "31 issues" and used that as the branch name rather than actually looking at the issue number. :(

@jamesarosen
Copy link
Owner

+1

@jamesarosen jamesarosen merged commit ce7779e into jamesarosen:master Sep 13, 2016
@swquinn swquinn deleted the gh-31 branch September 13, 2016 16:43
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