Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(tests): Use goog.bootstrap to allow loading ES modules in playground #5931

Merged
merged 5 commits into from
Feb 23, 2022
Merged

refactor(tests): Use goog.bootstrap to allow loading ES modules in playground #5931

merged 5 commits into from
Feb 23, 2022

Conversation

cpcallen
Copy link
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

Part of #5904.

Proposed Changes

Change how tests/playground.html loads Blockly.

Behaviour Before Change

  • playground.html would use a <script> tag to load tests/playgrounds/load_all.js
    • which in turn would (in uncompiled mode) use a <script> tag to load blockly_uncompressed.js from the project root
      • which in turn used further <script> tags to load base.js and deps.js and then finally do a goog.require('Blockly') call to get the debug module loader to load core/*.
    • After this, load_all.js would use further <script> tags to load msg/messages.js, tests/themes/test_themes.js and @blockly/block-test.
  • Finally, a <body onload="..."> would call the start function in playground.html.

Behaviour After Change

  • playground.html will use a <script> tag to load tests/playgrounds/prepare.js
    • which in turn will (in uncompiled mode) use <script> tags to load base.js, deps.js, and msg/messages.js.
    • It will then create a Promise, stored on window.BlocklyLoader, that will:
      • Call goog.bootstrap(), passing in a list of the top-level entrypoints (Blockly, Blockly.blocks.all, Blockly.Javascript.all, etc.)
      • Copy messages loaded from msg/messages.js from where they were temporarily stored into Blockly.Msg.
      • Resolve to a value that is the Blockly module exports object.
  • playground.html will then use an inline <script type=module> to import tests/playgrounds/blockly.mjs,
    • which will await the resolution of the loader Promise, and export the returned value.
  • The inline module will then call start() directly, which is safe because the page will have been loaded by this point.

(In compiled mode, prepare.js will load blockly_compressed.js et al. using the same implementation as load_all.js did previously.)

Reason for Changes

Because the debug loader loads ES modules using <script type=module> tags, their loading is deferred until after all the top level (non-module) <script> tags have been run. This means that any ES modules (and any goog.modules that depend on them, including the root Blockly module) will not be loaded in time to call Blockly.inject from the start script.

Test Coverage

Tested on:

  • Desktop Chrome

Also passes npm test.

Documentation

Once the dust has settled, we probably ought to update any documentation that we have that discusses loading Blockly in uncompiled mode since any previous sample code snippets will not work.

Additional Information & questions

  • This only addresses tests/playground.html, not any of the other playgrounds, tests, samples, etc.
  • Earlier I was encountering difficulty with blocks modules' initialisation code being run before Blockly.Msg had been populated, but moving the goog.bootstrap call from blockly.mjs (executed after page has loaded) to prepare.js seems to fixed this problem as well.
    • It may however reoccur when blocks modules's dependencies are migrated to ES module, causing both the dependency and the dependent block module evaluation to be delayed until after the page has been loaded.
  • I have omitted loading tests/themes/test_themes.js and @blockly/block-test as these were not being loaded in compiled mode.
    • Do we want to load them in uncompiled mode?
    • Ought they also to be loaded in compiled mode?

@cpcallen
chore(deps): Update closure/goog/base.js, add goog.js
b1371ca 
* Update base.js from the latest version (20220104.0.0).

* Also copy over goog.js, which provides access to asuitable subset
  of goog.* via an importable module).
@cpcallen
refactor(tests): Have playground.html load Blockly as a module
d675b89 
N.B.:

* We still need a preparation step, in order to load base.js and
  deps.js via <script> tags in uncompiled mode; in compiled mode
  it will instead load all the *_compressed.js files via <script>
  tags.

  Acess to the Blockly object is via:

      import Blockly from './playgrounds/blockly.mjs';

  (N.B: no "* as", since blockly.mjs has only a default export.)

* There remain two serious defects when running in uncompiled mode:

  * It does not attempt to load msg/messages.js, causing startup to
    fail.
  * Module loading only works if there are no ES Modules; if there
    are, something goes wrong with base.js's attempt to sequence
    module loads causing goog.modules that import ES modules to get
    a null exports object for that import.  X-(
@cpcallen
fix(tests): Have playground.html load messages.js before generators
50aefdd 
This fixes the issue caused by missing messages when loading
the generators.
@cpcallen
fix(tests): Move bootsrap calls to prepare.js
a759d95 
Move the calls to goog.bootstrap from blockly.mjs to prepare.mjs.
This is needed to work around a bug in the Cosure Library debug
loader (google/closure-library#1152).

This gets a bit ugly because most of the code has to go in a
<script> (because it needs goog.bootstrap, which was loaded by
an earlier <script> tag).
@cpcallen cpcallen added component: tests PR: chore General chores (dependencies, typos, etc) labels Feb 12, 2022
@cpcallen cpcallen requested a review from a team as a code owner February 12, 2022 00:21
rachel-fenichel
rachel-fenichel reviewed Feb 12, 2022
View reviewed changes
closure/goog/base.js Show resolved Hide resolved
closure/goog/goog.js
// limitations under the License.

/**
* @fileoverview ES6 module that exports symbols from base.js so that ES6
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sweet, I think this will make the typescript compiler happier. Did you copy it from closure, or write it yourself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes: this is copied straight over. I didn't end up using it in this PR, but we will need it to do goog.declareModuleId in ES modules, if we are to comply with the recommended approach.

tests/playgrounds/blockly.mjs Outdated Show resolved Hide resolved
tests/playground.html Outdated Show resolved Hide resolved
tests/playgrounds/blockly.mjs Show resolved Hide resolved
tests/playgrounds/prepare.js Show resolved Hide resolved
@rachel-fenichel
Copy link
Collaborator

rachel-fenichel commented Feb 12, 2022 •
edited
Loading

This only addresses tests/playground.html, not any of the other playgrounds, tests, samples, etc.

The only other file that uses load_all.js is the multi-playground, so cleaning that up should be a fast follow.

I have omitted loading tests/themes/test_themes.js and @blockly/block-test as these were not being loaded in compiled mode.

@alschmiedt are we getting any value from the test themes at this point? Are they part of our testing steps? If not, let's remove them.

@cpcallen cpcallen merged commit b1f0a6a into google:develop Feb 23, 2022
@cpcallen cpcallen deleted the load-esm branch February 23, 2022 10:30
@cpcallen cpcallen mentioned this pull request Mar 3, 2022
Closed
@BeksOmega BeksOmega mentioned this pull request Mar 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rachel-fenichel rachel-fenichel rachel-fenichel approved these changes

@alschmiedt alschmiedt Awaiting requested review from alschmiedt

Assignees

@rachel-fenichel rachel-fenichel

@alschmiedt alschmiedt

Labels
component: tests PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cpcallen @rachel-fenichel @alschmiedt