fix: message loading from script tags

[maribethb]

The basics

• I branched from develop
• My pull request is against develop
• My code follows the style guide
• I ran npm run format and npm run lint

The details

Resolves

Possibly related to #4369. Fixes google/blockly-samples#1066 and fixes google/blockly-samples#1065

Proposed Changes

Removes redundant code from packaged msg files.

Behavior Before Change

If you load a message file from dist/msg in a script tag, or equivalently if you load a message file from the blockly node module via a script tag (such as we do in samples, or what you would do if you used unpkg) then the message references do not work and you get errors like No message string for %{BKY_TEXT_APPEND_TOOLTIP} in %{BKY_TEXT_APPEND_TOOLTIP} and your blocks are broken.

Behavior After Change

Message loading works correctly.

Reason for Changes

Rachel and I investigated this bug after realizing this issue has been present on some of the blockly-samples examples for a while. We are not 100% sure on the cause of this bug but we think this may be the cause based on some evidence:

• this line of code has been here for more than two years, and possibly since Sam wrote the initial wrapper scripts.
• the logic around message loading was changed significantly during the move to closure modules e.g. https://github.com/google/blockly/pull/5768/files but we were not adequately testing the packaged files (Need a test that exercises the packaged files #6059)
• this has been broken in blockly-samples since at least v7 (ie this is not a new issue introduced in v8)
• removing this line fixes the problem :)

Test Coverage

Tested various scenarios. These worked before and continue to work:

• Loading blockly playground in core (npm run start)
• Loading the mirror demo in core as it currently is (references the non-packaged file)
• Loading a plugin that uses Blockly and the packaged files via modules and webpack (e.g. continuous-toolbox)

These were broken before this change but now work:

• Loading the mirror demo in core when it uses the packaged message file instead of the non-packaged one
• Loading the mirror demo in blockly-samples
• Loading the plane demo in blockly-samples

I would like to test with angular or react to see if #4369 is resolved but still working on that.

Documentation

Additional Information

Our current plan is to release a beta version with this change, and ask some folks that seemed affected by this issue to test with the beta. If it works, and if Christopher agrees, then we can release a patch version of Blockly with this change.

We can also use the beta to update the blockly-samples gh-pages site since that is currently blocked by this issue.

We want to wait for Christopher's opinion as the resident expert on module loading.

[rachel-fenichel]

@cpcallen fyi

[cpcallen]

So, my thoughts:

• You are correct that the bug is caused by a change introduced during the move to goog.module, and I believe you are correct about which one. AFAICT the full picture is as follows:

  • 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 b.

  • 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.

• Your PR will definitely fix the problem.

• But it 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 currently do did prior to fix: message loading from script tags #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.

• And 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.

[maribethb]

Thank you for the write up! I should have thought to test with Blockly.setLocale. My vote would then be that we revert this PR, and make #6123 a P0 for this quarter. As a note if we revert this PR then we won't be able to update demos in blockly-samples until that issue is resolved, but I don't want to accidentally merge this into a real release.