Latest patch (Q4 2021 Patch 3) fails to load

[KlemenDEV]

Describe the bug

The latest patch (Q4 2021 Patch 3) fails to load

To Reproduce

Try to load Blockly as specified with examples.

Resolution

This seems to be fixed by changing line from

blockly/blocks_compressed.js

Line 10 in caba596

root.Blockly.blocks.all = factory(root.Blockly);

to

root.Blockly.blocks = factory(root.Blockly);

[Taikono-Himazin]

Thanks to this, it works.
thank you.

[alschmiedt]

Thanks for reporting! We are currently working on a fix for this, but in the meantime we have reverted the patch so version 7.20211209.4 should work as expected.

[cpcallen]

O.K., I came up with a fix for this issue yesterday—for which I will submit a PR shortly—but I realised I was confused about why it should even have been necessary so in the mean time I advised @alschmiedt to submit #5938 to revert the release until I could be sure that my fix was actually correct.

Here's the deal. As noted in this (and other) bug reports, the error is indeed caused by the line

root.Blockly.blocks.all = factory(root.Blockly);

in blocks_compressed.js.

My first take was: of course this will fail, because while blockly_compressed.js, when loaded in a browser, creates a root.Blockly object that object won't have a .blocks property onto which could be tacked an .all. I prepared a fix based on this.

But then, while verifying my fix, I noticed / remembered that, because of how .declareLegacyNamespace works (specifically, in this case, how it is implemented by Closure Compiler), the factory function actually creates a .blocks property and initialises it with an empty object and then additionally creates the .blocks.all property and initialises that too! So root.Blockly.blocks.all = factory(...) should in fact be doing nothing at all. (If you look at the contents of blocks_compressed.js, note that root.Blockly will have previously been set to $.Blocky and factory returns $.Blockly.blocks.all.)

Unfortunately, while root.Blockly.blocks.all = factory(root.Blockly) ought to do nothing, it is actually throwing TypeError, because root.Blockly.blocks is evaluated before the call to factory which would create the .blocks property, and thus evaluates to undefined, and attempting to assign to (undefined).all is indeed a TypeError.

Here ends today's lesson on the order of evaluation of JavaScript expressions.

[cpcallen]

I've also looked in to why this issue was not picked up in testing. It turns out that:

• The playgrounds have been loading only blockly_uncompressed.js via <script>, then use goog.requireing to load the other modules they need.
• The mocha tests only import blockly_compressed.js, not any of the other compiled chunks.
• The node tests import all of dist/, but this exercises only the second clause of the UMD wrapper:

  ;(function(root, factory) {
    if (typeof define === 'function' && define.amd) { // AMD
      define(["./blockly_compressed.js"], factory);
    } else if (typeof exports === 'object') { // Node.js
      module.exports = factory(require("./blockly_compressed.js"));
    } else { // Browser
      root.Blockly.blocks.all = factory(root.Blockly);
    }
  }(this, function(Blockly) {
    /* ... */

  which does not trigger the bug in the third clause.

In PR #5835, @alschmiedt has somewhat rectified this by having the playgrounds load blocks_compressed.js via a <script> tag in certain instances, but this will still not exercise the relevant code in the most typical tests that we do during development, and I am not aware of us doing any testing, ever, that exercises the AMD clause at the top of the wrappers, so I have filed #5944 to address this deficiency.

[alschmiedt]

Closing since the fix to this was releasing version 7.20211209.4 and the fix to the bug causing the patch to fail was fixed in #5953.