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

destructure Ember.I18n each time render is called #436

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iezer
Copy link

@iezer iezer commented Dec 1, 2016

Fixes using ember-i18n.

Otherwise the workaround described in https://github.com/DockYard/ember-validations/issues/366#issuecomment-169869004 no longer works. When running multiple tests, ember-validations will use a destroyed version of i18n from the 1st test run, leaking memory in the process. CC @jamesarosen

Do not destructure before exporting.

Fixes using `ember-i18n`.

Otherwise the workaround [described
here](https://github.com/DockYard/ember-validations/issues/366#issuecomment-169869004)
using `ember-i18n` does not work. When running multiple tests,
ember-validations will use a destroyed version of `i18n` from the 1st
test run.
@bcardarella
Copy link
Contributor

👍

@bcardarella
Copy link
Contributor

Actually, let me back up. I might be following this incorrectly. So Ember.i18n gets recreated each time?

It might make more sense to inject the service instead rather than destructure from the const

@iezer
Copy link
Author

iezer commented Dec 1, 2016

Yes injecting the service would be better. We have a hack in an instance-initializer to set Ember.I18n = application.container.lookup('service:i18n')

@jamesarosen
Copy link

In older versions of ember-i18n, there was a fixed global, window.Ember.I18n. That's the interface that this library built around.

Later, ember-i18n switched to service:i18n as the core entry point.

@iezer referenced a comment I made a while back on how to fake the old interface from the new (to support this library with newer ember-i18n), but that relies on setting up and tearing down window.Ember.I18n for each test. (If you don't do that, then you leak references to the container into the global namespace, which will quickly cause your test suite to run out of memory.)

@jamesarosen
Copy link

jamesarosen commented Dec 1, 2016

It would be lovely if this library were to rely on service:i18n, but that means here can't be any static code that requires internationalization.

For example, addon/messages.js will have to become a service:

// ember-validations/app/services/validation-messages.js
import Ember from 'ember';
import getOwner from 'ember-getowner-polyfill';

export default Ember.Service.extend({
  // i18n: Ember.inject.service(), // can't do this because service:i18n might not exist

  render(attribute, context) {
    const i18n = getOwner(this).lookup('service:i18n');
    if (i18n) { return i18n.t(`errors.${attribute}`, context);
    ...
  }
});

That, in turn, means that the various validators have to change to get the service:validation-messages:

// ember-validations/addon/validators/local/numericality.js
import Ember from 'ember';
export default Base.extend({
  validationMessages: Ember.inject.service(),

  init() {
    ...
    this.options.messages.numericality = this.get('validationMessages').render('notANumber', this.options);
  }
});

@bcardarella
Copy link
Contributor

It would be lovely if this library were to rely on service:i18n

confirm, I have been planing a rewrite for 2.0. I may reclassify this as a defunct 2.x attempt (we never left alpha for 2.x) and that would be a better place for hooking into i18n correctly. Odds are I would just leave it out of this library and up to the consumer to enable.

@jamesarosen
Copy link

How do you feel about shipping this in the meantime?

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