Fix language loading problems

[cpcallen]

@Maribeth created PR #6060 to solve a problem with language loading. The problem was that, prior to that PR:

• The wrappers generated for dist/msg/*.js:

  1. pass in the Blockly object to the factory function,
  2. (until this PR) immediately shadowed this value with a Blockly local variable set to {Msg: {}},
  3. return this fresh Blockly.Msg object from the factory function,
  4. and finally overwrite the (global) Blockly.Msg with the Msg object created in step 2.

• Overwriting the global Blockly.Msg object with a new object was really wrong but pretty much worked:

  • In thegoog.provides days all accesses to this dictionary were via the global Blockly.Msg, so they would see the replacement object.
  • Prior to refactor: core/msg.js: use named export, remove declareLegacyNamespace #5768, closure compiler would compile accesses to the Msg dictionary object into $.Blockly.Msg.MESSAGE_NAME, and since $.Blockly was the global Blockly object these would see the replacement object. Oh yes: and the compiler doesn't bother running Object.freeze on module export objects, so the overwrite would not blow up.
    *After refactor: convert some block generators to goog.module #5769, in uncompiled mode, the exports object of Blockly.Msg was frozen, but the .Msg property on the core/blockly.js exports object was not. Overwriting Blockly.Msg would have had no effect (no messages would have been loaded in to the dictionary object used by core/ code)—but uncompiled mode users (e.g. the playground) load messages from msg/messages.js which does not have the wrapper code that does this overwrite but instead just sets individual properties on the Blockly.Msg object (which is presumed already to exist), so everything worked fine here too.

• After refactor: convert some block generators to goog.module #5769, closure compiler compiles accesses to theMsg dictionary object into $.module$exports$Blockly$Msg.Msg.MESSAGE_NAME, which would not have been affected by overwriting Blockly.Msg (which would only set the value of $.module$exports$Blockly.Msg, not used by any core code)—and so message loading fails.

PR #6060 will fix the immediate problem, but may have some unintended consequences:

• I think that—at least back in the goog.provides days—it may have been possible to load the language file first, then load Blockly, and have everything work. That has probably not been the case for a while, but it will definitely not be the case now.
• Until now, the wrapped dist/msg/*.js files could be loaded as CJS modules just for their exports (though they have the unfortunate side effect of not just overwriting Blockly.Msg as noted), but now—though they will continue to export the (original, unique and correct) Blockly.Msg dictionary object, it will no longer be possible to use Blockly.setLocale to switch between languages, because if you load multiple language files (e.g. by using require in Node.js), each one will not only set Blockly.Msg (as one would want) but would thereby overwrite the properties on the common "exports object".

The latter is probably going to upset some folks.

So I think we need to find a way of having the language files do two of the three thing they did prior to #6060:

1. Each have a separate exports object, so you can usefully load more than one at a time.
2. Ensure that the messages defined are loaded in to Blockly.Msg as a side effect of the language file being loaded.
3. Replace the Blockly.Msg dictionary with their export object.

I think this should be done with a view to eventually making the language files side-effect free, as we are doing with the library blocks. Further, I think it would be preferable, if possible, to put as much of the code to effect all this in files generated in msg/js/*.js, which could perhaps become actualgoog.modules (or failing that at least in the "compiled" files in build/msg/js/*.js) rather than in the wrappers added in dist/, if for no other reason than to help avoid problems like this going unnoticed so easily.

Originally posted by @cpcallen in #6060 (comment)

[BeksOmega]

Ok, I think I understand the issue here.

Modules Background

Inside blockly_compressed.js we have the module junk closure outputs: $.module$exports$Blockly$whatever.whatever. Then we also have the backwards-compatible blockly tree that we've constructed: $.Blockly.whatever, which refers to the $.module$exports$Blockly$whatever.whatever stuff.

For example:

$.Blockly.Msg = $.module$exports$Blockly$Msg.Msg;

The global root.Blockly that we get whenever we import Blockly (in any way) is the $.Blockly tree.

Before we moved to goog.modules we did not have module junk. Before we would just have one Blockly tree. So the Blockly in blockly_compressed was global.Blockly, root.Blockly, etc, whatever you want to call it. It was just one thing. But now we have the tree, and all of the module junk.

And now, things inside compressed only look at the module junk not the tree.

Timeline

So in the "just one tree period" of Blockly, when we did root.Blockly.Msg = factory(root.Blockly) The root.Blockly passed to factory was not being used. factory was creating a new object, and then that got assigned to root.Blockly.Msg. This worked, because everything in blockly_compressed was also looking at root.Blockly.

But now in the "tree + module junk period" of Blockly, when we did root.Blockly.Msg = factory(root.Blockly), we were overwriting the tree, but not the module junk. So everything in blockly_compressed would still be refering to the old $.module$exports$Blockly$Msg.Msg object. This was what caused google/blockly-samples#1066, google/blockly-samples#1065, etc.

Then after #6060 root.Blockly.Msg = factory(root.Blockly) factory would no longer create a new object. It would just modify root.Blockly.Msg directly. Remember root.Blockly == $.Blockly and root.Blockly.Msg == $.Blockly.Msg == $.module$exports$Blockly$Msg.Msg. So when we modify root.Blockly.Msg inside factory we are modifying the tree and the module junk Yayayay!

However, because factory is no longer creating a new object, this means that when we return Blockly.Msg at the bottom of factory we are just returning root.Blockly.Msg, $.Blockly.Msg, $.module$exports$Blockly$Msg.Msg, whatever you want to call it. So say we're loading some CJS modules:

var spanish = rquire('blockly/msg/es');
var french = require('blockly/msg/fr');

This means that spanish == french == root.Blockly.Msg == $.Blockly.Msg == $.module$exports$Blockly$Msg.Msg which sucks, because we want spanish != french. This is the problem Christopher recognized in #6060 (comment).

Solution

My suggestion is that:

1. factory should return a new object, so that spanish != french.
2. Inside the // Browser route of the UMD, instead of doing root.Blockly.Msg = factory(root.Blockly) we do Object.assign(root.Blockly.Msg, factory(root.Blockly). So that root.Blockly.Msg, $.Blockly.Msg, $.module$exports$Blockly$Msg.Msg, whatever you want to call it, gets the messages we want.
3. We should stop passing root.Blockly to factory because it isn't even used, and it's super confusing.

[BeksOmega]

We've submitted a fix for this that we hope should work for all methods of loading (module systems, and script tags). For anyone following this issue, could you try this beta version: 8.0.4-beta.0 and reply here if it works or not?

If we get feedback that it works, then we can release an actual patch for the wider world!

Thanks,
Beka