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

Remove Closure library and module system from the Blockly codebase? #6858

Closed
19 of 20 tasks
cpcallen opened this issue Feb 21, 2023 · 1 comment
Closed
19 of 20 tasks

Comments

@cpcallen
Copy link
Contributor

cpcallen commented Feb 21, 2023

This is a discussion + tracking bug for work to remove our remaining dependencies on the Closure library (of which closure/goog/base.js and …/goog.js are the only remaining pieces) and the Closure module system (goog.module / goog.require).

The existence of this bug does not denote that a decision to remove the Closure library from our codebase; it exists merely to document what would be involved in doing so and track what work is done in that direction, as well as to provide a place to discuss the merits of such work.

Background

Terminology

There are several related but separate Google products named "Closure":

Overview, Motivation and Limitations

Use of the Closure module system ties us to base.js for uncompiled-mode loading and to Closure Compiler for bundling. Since we'd like to try alternative bundlers such as Webpack, it would be good to remove our remaining use of the module system.

Even once we have fully removed use of the Closure module system we may wish to retain use of the debug module loader (in base.js) at least in the short term because it can do useful things like load JS scripts (via <script> tags) after ES modules have been imported.

We will probably wish to continue to use Closure Compiler at least until an alternative bunding/minification tool has been proven to be a good alternative. This is likely to entail our continued use of closure-calculate-chunks which depends on closure-make-deps which in turn expects the existence of at least a minimal base.js.

Work already completed

(…as of the creation of this bug, late February 2023)

  • Almost all of use of the Closure module system (goog.module etc.) in core/ was retired with PR chore: Migrate core/ to Typescript, actually #6299 (see also issue Update Blockly codebase to Typescript #5857), but we still use it in various places, notably in blocks/ and generators/.
  • We've broken all import cycles amongst modules in core/.
    • closure-make-deps enforces a rule against circular goog.requires because the semantics of Closure modules do not allow these.
    • Circular imports amongst ES modules are in general perfectly legal, although they can make module loading "fragile": particularly where a class in one module in a cycle extends a class defined in another module in that same cycle, it matters which module is imported first.
    • closure-make-deps does not enforce a rule against import cycles, but the debug module loader (somewhat accidentally) does in some cases, due to assertion check in its simplistic implementation of goog.declareModuleId.
    • Because there is no equivalent of goog.module.get for ESM modules, we introduced a number of import cycles while converting core/ from Closure modules to TypeScript modules.
    • We fixed most of these by using monkey-patching to do dependency injection, and then deliberately disabled the assertion that was catching the remaining ones, in PR fix: circular dependencies #6281 (which became commit 9d5dcc6 of PR chore: Migrate core/ to Typescript, actually #6299).
    • The last remaining cycle was fixed in refactor: Remove last remaining circular import in core/ #6818.

Remaining Work

Convert remaining goog.modules to ES Modules (or TypeScript modules)

We almost certainly want to do all of this, as these are blockers for trying Webpack.

  • blocks/* and generators/*: tracked (as part of the migration to TypeScript) in Convert blocks/ and generators/ to TypeScript #6828.
  • core/main.js: most of this is deprecated get/set acccessors which can be deleted, but here may be some technical challenges converting any remaining code to TS or ESM because:
    • this file is our main entry point (for deps, chunking, and compilation), and
    • it was created as a refuge for code we didn't want to convert to TS when we converted the rest of core/.
  • The advanced compilation test (which we may also wish to convert to TypeScript at the same time):
    • tests/compile/test_blocks.js
    • tests/compile/main.js

Remove other remaining uses of goog.require

These are relatively straight forward to do once we no longer have any goog.modules.

  • Convert remaining mocha tests to use import:
    • tests/mocha/index.html
    • tests/mocha/field_multilineinput_test.js
    • tests/mocha/generator_test.js

Deprecate bootstrap.js and the debug module loader

  • Modify tests/bootstrap.js and helpers to use import instead of goog.require. This may involve a substantial rewrite, if we cannot continue to use goog.bootstrap. Replace tests/bootstrap.js:
    • Create shims: ES modules that will either import the chunk entry point or use a <script> tag to load the compressed chunk, and then (in either case) export the chunk's public API.
    • Convert tests/playground.html to use shims.
    • Convert tests/multi_playground.html to use shims.
    • Convert tests/playgrounds/advanced_playground.html to use shims.
    • Convert tests/mocha/index.html to use shims.
    • Convert tests/generators/index.html to use shims.
  • Delete tests/bootstrap.js et al.

Remove Closure library

This is lower-priority work, but would let us remove a bunch of cruft from our codebase and build pipeline.

Cleanup

@cpcallen
Copy link
Contributor Author

cpcallen commented Sep 8, 2023

All remaining work has been split out into individual issues.

@cpcallen cpcallen closed this as completed Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants