Make output code independent of all globals

[eemeli]

This fixes the issues raised here by making the one-letter utility functions properly local to the output, rather than leaking into the global namespace.

This is done by adding a new function MessageFormat.LocalFunctions(), which returns said function declarations as a string that's included by compile() in lib/messageformat.dev.js as well as build() in bin/messageformat.js. In the former, the functions are included within the scope of the new Function() -generated function; in the latter, they're scoped within the wrapping anonymous function.

As a side product, lib/messageformat.include.js is now obsolete and hence removed; this is good because it means that the utility functions are only defined in one place.

This also enabled me to remove references to MessageFormat.locale from the generated code, as they're now wrapped up by MessageFormat.LocalFunctions().

If this pull request is merged, MessageFormat.prototype.compile() will always return a function that's independent of all globals, including MessageFormat itself. However, to write that function to disk for later use, you'll need to also write the output of MessageFormat.LocalFunctions() to a namespace accessible from the same place, just like build() does.

While I was poking at the code, I also simplified things a bit by inlining the utility functions that are each only used once and could be replaced by single lines of code. As a result, fallbackLocale, pluralFunc, SafeString and Utils have been removed.

[SlexAxton]

So this puts all those functions in every single compiled function? Wouldn't it make more sense to just reference them on the MessageFormat object?

[SlexAxton]

Oh, it doesn't do per function, it wraps the group of all built messages with a set of the functions. That seems fine if you use the messageformat cmd-line stuff, but still think it'd be fine to just add them to MessageFormat.runtime or whatever.

[eemeli]

The problem with that approach is that MessageFormat the global doesn't exist in the compiled code, and the utility function calls really ought to be the same in all cases. So either we need to use local functions like I'm suggesting, or we need to use some way of generating a trimmed-down version of MessageFormat that'll then need to be included along any generated code. Which I understand you'd like to be a global, but with functions that only make sense for the generated functions, yes? In other words, a more verbose way of doing the same thing.

Maybe we're talking a bit past each other here, as we're clearly using the library in different ways. Could you point me at some pre-existing Handlebars or other extension that uses messageformat.js, so I could see how the code is used there? That might help me to understand what you're looking for.

[SlexAxton]

Sure.

So I do two things:

<div>
{{#mf "thekey" .}}
There are {count} results.
{{/mf}}
</div>

I parse the AST of the Handlebars templates, and find instances of mf blocks. So when I compile my Handlebars templates, it ends up looking more like this:

Handlebars.registerHelper('_mf_thekey', function(data) {
  var msg = function(d){ /* precompiled mf function */ };
  return msg(data);
});

<div>
{{_mf_thekey .}}
</div>

Technically I could output all of those registerHelper functions into a scope where I have access to the helper functions, but that is a little bit prescriptive for the open source tool I'm trying to put out to do this stuff.

---

Secondly, I can request messages as modules with an AMD plugin.

require(['mf!thekey'], function(msg) {
  console.log(msg({count: 5});
});

When I run this in dev mode, it's just compiling stuff on the fly and eval'ing. But when I run the optimizer across these plugins, they output static modules that look like this:

define('mf!thekey', ['messageformat'], function(MessageFormat) {
  return function(){ /* Precompiled messageformat func here */ };
});

In that case, I actually don't use the global, but internally in the old precompiled output, the function referenced a few properties on the MessageFormat variable that I required as a module. In the case of these few new functions we're using, it seems like that'd still very much work nicely.

define('mf!thekey', ['messageformat'], function(MessageFormat) {
  return function(){ ... MessageFormat.p(...); ... };
});

I end up not using the messageformat command line tool because, this allows me to only compile the messages that I'm explicitly requiring in my application into my build file.

There is not a pretty way to have the require.js optimizer wrap a set of defines in a function and throw a few functions at the top of them. I could do this by hand after the optimizer ran. I'd rather not, though.

[eemeli]

So if I've understood right, you need to either include everything inside each function call, or define a global somewhere. Can you define/modify that global after reading the template, or does it need to be static from the very beginning?

With something like that, you could read the template for all the mf blocks and use code like:

Handlebars.registerHelper('_mf_thekey', function(data) {
  return i18n['thekey'](data);
});

And for the static modules:

define('mf!thekey', ['messageformat'], function(i18n) {
  return i18n['thekey'];
});

Where p() & co. are local to the i18n object. Does that make sense?

We could generalise code from bin/messageformat.js a bit to make it more useful to cases like this; it should be pretty easy to get a function out of it that'll take in an associative array of MessageFormat strings---either read from file as the CLI tool does, or parsed from Handlebars templates or elsewhere---and return something that's a complete, storable object that has all the translation functions included.

Or we could change all the internal refs to point to something like i18n.p() rather than p(), which might indeed be simpler from an implementation point of view, if there's a need to be able to add to a previously constructed translation object rather than constructing it at once. The name of that object would need to get passed in as a parameter, though, and we'd therefore still need something like MessageFormat.LocalFunctions to generate it.

Hmmm... It might still be possible to support the model currently used in the CLI output with such a scheme... It gets a bit tricky sometimes, writing JavaScript that outputs JavaScript that may or may not be immediately executed, or it may get written to disk for later use.

Btw, about the gzipped size; you're right that the cost is minimal if all the functions are prefixed with MessageFormat.. Testing manually the increase is only about 2%, while for un-gzipped I get about 15%.

[SlexAxton]

(sorry for long delay, just got back on a 3 week trip)

Hey @eemeli let's compromise on this maybe? Let's keep in the closure style LocalFunctions call, but also use that function to generate a messageformat.runtime.js that contains just the few things that are needed to use MessageFormat at runtime. This is exactly like most templating languages do and I think it's a common enough pattern to use. I don't think we should put it on the i18n namespace, because that generally doesn't exist if you are using the runtime. Let's just put it on the Messageformat object or whatever.

Seem crazy? Too much? Good compromise?

[eemeli]

Actually, I sort of came around to your point of view, after getting rather deep into how closures work in JS, and how adding something to a closure after it's been, well, closed, just doesn't work. I've not worked on this in about two weeks, but I've a nearly ready branch that should fix this in a way that makes everyone happy.

What I've done there is add a function for now called MessageFormat.prototype.rootCode() that returns javascript code as a string, for use by MessageFormat.prototype.compile() as well as build() in bin/messageformat.js; this allowed for the removal of a separate lib/messageformat.include.js. I've also removed the duplicated locale loading from the CLI, and adjusted the locale function definitions to match the new usage. There's also a definable rootName to use for the generated object.

But like I said, that branch requires some cleanup and now re-basing, so shouldn't rely on it yet.

[eemeli]

I've submitted pull request #51, which replaces this one.