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.

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

refactor(tests): Introduce loading shims, use in playgrounds #7380

Merged
merged 9 commits into from
Aug 14, 2023

Conversation

cpcallen
Copy link
Contributor

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

Part of #6858

Proposed Changes

  • Simplify implementation of posixPath in scripts/helpers.js

  • Make safe to import closure/goog/goog.js in node.js.

    Modify the implementation of declareModlueId so that it can be safely (albeit ineffectively) imported and executed by node.js, which does not provide a window global.

  • Introduce chunk loading shims:

    • Add a buildShims task to build_tasks.js that, for each chunk, creates a correspondingly-named build/<chunk>.mjs that will either (in uncompressed mode) import and reexport that chunk's entry point module (e.g. core/blockly.js) or (in compressed mode) load dist/<chunk>_compressed.js using a <script> tag and then export the corresponding properties on the chunk's exports object.

    • Provide helper methods used by these shims in tests/scripts/loading.mjs, including code to detect whether to load in compressed or uncompressed mode.

    • Add a quote() function to scripts/helpers.js, used by buildShims. This is copied from tests/bootstrap_helper.js, which will be removed in a later commit.

  • Update playground.html, advanced_playground.html and multi_playground.html to use the new loading shims.

  • Delete playgrounds/shared_procedures.html: shared procedure support was moved to a plugin and this should have been removed from core along with it.

Behaviour Before Change

The playgrounds used tests/bootstrap.js et al. to load Blockly and other needed scripts, ultimately depending on the debug module loader in closure/goog/base.js to do the actual loading.

Compressed/uncompressed selection was done by looking for localhost and/or file: in the URL, and could be overriden only by having the playground set an option on the BLOCKLY_BOOTSTRAP_OPTIONS global options object before loading bootstrap.js.

Once loaded, Blockly was made available via the Blockly global variable; generators via javasscriptGenerator (etc.) globals.

Behaviour After Change

The playgrounds can import * as Blockly from '…/build/blockly.mjs'; (and similarly for blocks, generators)—almost as if loading it from the blockly NPM package.

It is possible to select compressed vs. uncompressed mode by appending a ?compressed=true (or false, or yes/no or 1/0) query parameter to the URL.

The mocha and generator tests still use bootstrap.js for now; they will be converted (and the bootstrap code removed) in a future PR.

Reason for Changes

  • Reduce dependency on Closure debug module loader.
  • Make importing of Blockly in playgrounds more consistent with how this is done in blockly-samples.
  • Maintain ability to choose compressed/uncompressed mode, with a more convenient way to override default choice.

Test Coverage

Unchanged. Playgrounds tested manually (resulting in incidental discovery of issue leading to revert of PR #7333 by PR #7375 and swift update of Blockly v10.1.0 with v10.1.1).

Documentation

The ?compressed= parameter should probably be documented somewhere, but I'm not sure where the best place would be.

Additional Information

I considered implementing the compressed/uncompressed switching by using import maps to choose between two different modules:

  • In compressed mode, the 'blockly' module (e.g. import … from 'blockly';) would be mapped onto an ESM much like the shims created here, that would load the <chunk>_compressed.js and reexport the properties on its exports object, but slightly simplified by not have any support for loading in uncompressed mode.
  • In uncompressed mode, 'blockly' would be mapped directly to build/src/core/blockly.js.

However there are two reasons I did not go down that route:

  • It would entail changing import maps based on window.location, and it wasn't completely clear how easy that would be to do. Certainly it would entail having an initial <script> run to set up the correct import map before a <script type="module"> could do the actual imports, whereas the approach chosen hides all the choosing logic in the shims themselves (or rather in load.mjs, which they all depend on).
  • Import maps are a pretty new feature, only recently supported by all the major browsers and not yet supported by all.

It may well make sense in a year or two to add a (static) import map so that import * as Blockly from './path/to/build/blockly.mjs' can be simplified to import * as Blockly from 'blockly', while leaving the shims as they are.

Make the implementation declareModlueId safe to import and
execute in node.js, which does not provide a window global.

(N.B. because this is an ESM and therefore automatically
strict, using window?.goog?.declareModuleId doesn't work
because window being undefined is an early error.)
- Add a buildShims task to build_tasks.js that, for each chunk,
  creates a correspondingly-named build/<chunk>.mjs that will
  either (in uncompressed mode) import and reexport that chunk's
  entry point module (e.g. core/blockly.js) or (in compressed
  mode) load dist/<chunk>_compressed.js using a <script> tag
  and then export the corresponding properties on the chunk's
  exports object.

- Provide helper methods used by these shims in
  tests/scripts/loading.mjs, including code to detect whether
  to load in compressed or uncompressed mode.

- Add a quote() function to scripts/helpers.js, used by
  buildShims.  This is copied from tests/bootstrap_helper.js,
  which will be removed in a later commit.
Shared procedure support was moved to a plugin and this should
have been removed from core along with it.
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Mostly just typos :P and then one actual question!

I feel like this system fits in my head so much better than whatever the heck the debug module loader stuff is doing :P so happy!!

tests/scripts/load.mjs Outdated Show resolved Hide resolved
tests/scripts/load.mjs Outdated Show resolved Hide resolved
tests/scripts/load.mjs Outdated Show resolved Hide resolved
tests/scripts/load.mjs Outdated Show resolved Hide resolved
scripts/gulpfiles/build_tasks.js Outdated Show resolved Hide resolved
cpcallen and others added 2 commits August 14, 2023 21:30
Per suggestion on PR google#7380, have buildShims name the shims
${chunk.name}.loader.mjs instead of just `${chunk.name}.mjs`.
@cpcallen cpcallen merged commit 5b5a565 into google:develop Aug 14, 2023
7 checks passed
@cpcallen cpcallen deleted the refactor/6858/shims branch August 14, 2023 21:47
cpcallen added a commit to cpcallen/blockly that referenced this pull request Aug 16, 2023
In PR google#7380 it was suggested[1] that the shims be renamed from
(e.g.) blockly.mjs to blockly.loader.mjs, and in commit 6f930f5
this was duly done, but alas one place was overlooked.

The problem was not spotted in local testing because the
blockly.mjs module that the blocks and generators chunks were
attempting to import did still exist on disk, left over from
before the change was made.

Running npm run clean would have revealed the issue but alas
that was not done.

[1] google#7380 (comment)
cpcallen added a commit that referenced this pull request Aug 16, 2023
In PR #7380 it was suggested[1] that the shims be renamed from
(e.g.) blockly.mjs to blockly.loader.mjs, and in commit 6f930f5
this was duly done, but alas one place was overlooked.

The problem was not spotted in local testing because the
blockly.mjs module that the blocks and generators chunks were
attempting to import did still exist on disk, left over from
before the change was made.

Running npm run clean would have revealed the issue but alas
that was not done.

[1] #7380 (comment)
@cpcallen cpcallen mentioned this pull request Aug 29, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants