Global state leakage

[jmerrifield]

Initially filed at twitter/twitter-cldr-npm#9, closed and filed here with the blessing of @arnavk

We run this module in a Node.js web server, that handles high traffic, and each request can potentially have a different associated locale. Since updating to 2.x we've started experiencing occasional instances of the wrong date/time formatting being applied. Digging a little deeper, I believe there to be a global state leakage issue with the current release. The following is from a Node.js repl:

> var t1 = require('twitter_cldr').load('en')
undefined
> var t2 = require('twitter_cldr').load('hi')
undefined
> new t1.DateTimeFormatter().format(new Date(), {type: 'full'})
'मंगलवार, 21 जून 2016 को 11:45:30 पूर्वाह्न UTC-08:00'
> new t2.DateTimeFormatter().format(new Date(), {type: 'full'})
'मंगलवार, 21 जून 2016 को 11:59:43 पूर्वाह्न UTC-08:00'

I would expect the DateFormatter of t1 to return an 'en' formatted date string.

At the moment, I'm working around it like this:

const loadTwitterCldr = _.memoize(function(locale) {
  delete require.cache[require.resolve('twitter_cldr/full/core')];
  return require('twitter_cldr').load(locale);
});

I'm wondering if you have any better suggestions. I can try to contribute a fix if you'd be open to that? My initial thoughts are that the main export should be a constructor or factory function that returns a fresh TwitterCldr 'instance' on every invocation, such that individual ones can have .set_data called independently.

[pchew-change]

@camertron - do you have any potential insight into/advice on this?

[camertron]

@pchew-change I believe Arnav was looking into this. Any status updates @arnavk?

[arnavk]

oh shoot! I totally forgot about this. @jmerrifield, were you able to work on a fix? If not, I can attempt it in the next week or so.

[arnavk]

So, I went down the route @jmerrifield suggested. in the main bundle template I added a factory method. Something like:

factory: ->

  TwitterCldr = {}
  {{> utilities}}
  {{{contents}}}

TwitterCldr = factory()

The problem with this is that mustache renders the contents of contents in place without maintaining the whitespace (as it should, to be fair). That effectively renders:

factory: ->

  TwitterCldr = {}
  {{> utilities}}
{{{contents}}}

TwitterCldr = factory()

This makes the factory useless, in the sense that it only creates a TwitterCldr object with utilities.

Now, I'm way out of touch with this stack. But here is what I have in mind now:

Solution 1: Change the build pipeline to render contents into a separate coffeescript file and load that inline like utilities. This feels like a hack, though.

Solution 2: Tackle the problem differently (still working on this)

@camertron @KL-7 @jmerrifield any other ideas/thoughts?

[camertron]

Hmm yeah I see the problem... If we want to make instances, why not make TwitterCldr a coffeescript class?

class TwitterCldr
  constructor: (locale) ->
    @load(locale)

{{> utilities}}
{{{contents}}}

I think that will work... all the stuff in {{> utilities}} and {{{contents}}} should know to not redefine TwitterCldr but instead just add stuff to it. As long as calling .load() still works, we should be golden. Thoughts @arnavk?

[arnavk]

That could work. I'll give it a shot