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

Refactor to Ember Octane #83

Merged
merged 5 commits into from
Nov 17, 2020
Merged

Refactor to Ember Octane #83

merged 5 commits into from
Nov 17, 2020

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Oct 22, 2020

This updates to ember-cli 3.22 and Ember Octane.

The main change is refactoring everything to ES6 classes. This required a few adjustments, as native ES6 classes have slightly different timing semantics for the constructor, which don't play well with extending a base class - so the current way of defining availableLocales does not work there properly.

To accomodate this, the configuration has been moved to config/environment.js (where we already had a single configuration option for jsonPath). You can now configure this addon like this:

ENV['ember-l10n'] = {
  // This is required
  locales: ['en', 'de'],

  // These are optional (defaults listed)
  defaultLocale: 'en',
  autoInitialize: false,
  defaultPluralForm: 'nplurals=2; plural=(n != 1);',
  jsonPath: '/assets/locales'
}

You should not need to extend from the service anymore in your application.

Nothing should have to change in your application code. The usage of the service/helpers/component remains the same.

As this is a breaking change, I would release it as 5.0.0. Migration should be relatively straightforward.

Other notable changes in the move to Octane:

  • Use Glimmer component for <GetText> - this is an internal change, the usage did not change at all.
  • Avoid using observers for the helpers - this is an internal change, we now manually register callbacks with the service for when the locale changes. Functionally, nothing changes here.
  • Make autoInitialize default to false. In most scenarios you want to manually set the locale in the application route's beforeModel hook or similar, in order to ensure that the locale file is loaded before the application is shown.
  • The deprecated ember-l10n/test-helpers.js file is removed. It is not needed anymore for quite some time.

You might also need to adjust something in your tests, if you relied on overwriting some properties of the l10n-service there. You can do so now like this:

class ExtendedL10nService extends L10nService {
  _loadConfig() {
    let config = {
      locales: ['de', 'en', 'ko'],
      autoInitialize: false,
      defaultLocale: 'de',
    };

    return super._loadConfig(config);
  }

  _getWindow() {
    return {};
  }
}

In detail explanation on the timing semantics of ES6 classes vs. classic Ember classes

A short explanation on why the changes to the configuration where necessary in the move to ES6 classes follows.

Previously, we had an init hook like this:

init() {
    this._super(...arguments);

    this._checkLocaleFiles();
    // fastboot stuff

    if (get(this, 'autoInitialize')) {
      this.setLocale(this.detectLocale());
    }
  },

Now, the initial assumption would be that you can simply refactor this to:

constructor() {
  super(...arguments);
  
  this._checkLocaleFiles();
  // fastboot stuff

  if (this.autoInitialize) {
    this.setLocale(this.detectLocale());
  }
}

And at first, that does work as expected. However, this breaks when you try to overwrite default values on the class. Think this base class:

class L10nService extends Service {
  availableLocales = { en: 'en' };
  autoInitialize = true;
}

And this extended class:

class CustomL10nService extends L10nService {
  availableLocales = { en: 'en', de: 'de' };
}

Because of the way native ES6 constructors work, this will not work as expected. The reason is that the above code is internally resolved to this:

class CustomL10nService extends L10nService {
  constructor() {
    super(...arguments);
    
    this.availableLocales = { en: 'en', de: 'de' };
  }
}

Basically, anything set on an extended class will not be available in the base-class constructor, as the constructor of the base class is called first, and only then the "changes" of the extended class are applied. This is not Ember-specific but the way ES6 classes work generally.

This is the reason for moving the configuration out of the extended service into config/environment.js, which is probably cleaner anyhow (and also allows easily setting different settings for tests etc.).

@mydea mydea self-assigned this Oct 22, 2020
@mydea mydea force-pushed the fn/update-ember-cli-3.22 branch from 2abd5cc to e3f032f Compare November 2, 2020 07:12
@mydea
Copy link
Contributor Author

mydea commented Nov 3, 2020

I have checked this in two apps, one "regular" and one using fastboot. Works as expected!

It is better practice to manually inject the service where you need it.
* Use Glimmer component for <GetText>
* Avoid using observers for helpers
* Refactor to ES6 classes
* Refactor to use config from config/environment.js instead of extending service
@mydea mydea force-pushed the fn/update-ember-cli-3.22 branch from e3f032f to 87cce63 Compare November 10, 2020 06:57
@mydea
Copy link
Contributor Author

mydea commented Nov 10, 2020

If there are no objections, I will merge this tomorrow and cut a 5.0.0 release!

@mydea mydea merged commit 4c314b5 into master Nov 17, 2020
@mydea mydea deleted the fn/update-ember-cli-3.22 branch November 17, 2020 14:16
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.

1 participant