changeLocale resulting in upstream memory "leak"

[nathanhammond]

This results in a memory "leak" inside of moment.js: https://github.com/stefanpenner/ember-moment/blob/26d8a29/addon/services/moment.js#L45

The default empty object passed in changes the invocation signature here: https://github.com/moment/moment/blob/497f918/src/lib/locale/locales.js#L119

As a consequence, each invocation to updateLocale creates a new Locale which ends up with a reference to the previous Locale in parentLocale. If you do any configuration upon instantiation of an object (e.g. a Service) in a particular ApplicationInstance you end up with one leaked Locale per ApplicationInstance. (In normal apps, no big deal, FastBoot apps, things aren't good.)

Recommended Fix

There should be no default value of {} for the changeLocale function as the function that it proxies to has no default value. This modification in ember-moment effectively changes the upstream method signature.

Workaround

A user of ember-moment can pass a null or undefined second argument to MomentService#changeLocale.

[stefanpenner]

Ya, I can imagine this causing an issue for both fastboot + test runs. Good catch!

---

Just to make sure I'm following correctly. We essentially keep growing this object https://github.com/moment/moment/blob/497f918/src/lib/locale/locales.js#L13 because for each name variant, we create a new config. Which is inserted into moments global locales object?

If that is the case, the leak still occurs for people who switch locals if they provide their own config.

Given that, it would seem like we may need a way to delete entries from moments locals object, and we should do so when we delete the moment service (or something). Right?

[nathanhammond]

Basically, moment.js itself behaves "badly" if you continuously call their updateLocale function. In their defense it really doesn't make sense to call updateLocale very many times, especially for no-op reasons with {}. That method is intended to actually mutate the locale configuration, it just happens to have a side effect of switching to the locale you pass in.

So, given that, the problem lies in ember-moment: we conflate changeLocale with updateLocale. Most calls to changeLocale are users trying to get the side effect of getSetGlobalLocale as there is no way to otherwise sync MomentService.locale and moment.locale() in one call.

For guaranteed consistency you must instead do:

import moment from 'moment';

export default Service.extend({
  moment: service(),

  init() {
    moment.locale('en-gb'); // There is no way to reach through and do this from the service's public API.
    this.set('moment.locale', 'en-gb');
  }
});

[stefanpenner]

@nathanhammond can you think of any downsides to, if not lets just doit:

There should be no default value of {} for the changeLocale

cc @jasonmit

[nathanhammond]

Any user who is relying on MomentService#setLocale, MomentService#changeLocale, or MomentService#updateLocale to trigger a global moment.locale() change will be impacted in a non-obvious way. I'm not sure there is a 100% backwards-compatible change here.

We currently have two sources of truth for moment: configuration which is in the service, and configuration which is in the moment object returned by the module itself. I believe there is really just one good path forward: consolidate to a single invocation pattern. Either direction requires education and will be incredibly disruptive to large numbers of people.

1. You should never import moment from 'moment'; and instead always use MomentService#moment to get a moment instance. Stateless utility functions will have to have their moment instance passed to them. Remove the moment module.

2. Deprecate the moment service and always encourage import moment from 'moment';. Remove the moment service. Handle configuration in an initializer. Migrate all ember-moment-provided helpers to module-based use.

We elected for Option 2 in our codebase.

[jasonmit]

I'm all for option 2 as well.

Worth noting, we will lose the ability for the helpers to recompute on locale and timezone changes.