Review modules with side effects

[cpcallen]

Background

There are some modules within the Blockly codebase that have top-level code that consists not just of declarations and exports but which has side effects (typically calling methods in other modules).

This pattern is not forbidden by the style guide, nor necessarily bad practice, but it can have disadvantages and in some cases may be a code smell:

• It is impossible to import code or even type declarations from the module without invoking the side effects.
• The importer has little or no control over when the side effects occur, other than that they will have occurred before the importing module is itself executed.
  • And even that guarantee only holds if there is not a dependency cycle involving the two modules. Such cycles are forbidden by our style guide and the Closure Compiler, but are legal and often useful in other module systems including CJS and ES modules.
• Requiring the module multiple times does not cause the module body to be re-executed, so the side effects only get run once. (This is usually a feature, not a bug—but not always.)

Proposal

It is suggested that modules with side effects should be reviewed and consideration given to whether alternative patterns might be preferable:

• Modules that 'install' something (blocks, menu items, etc.) might be better to export an installThings method that could be called by the importing code—especially if the module exports some other symbols that an importer might wish to use without causing the installation to occur.

On the other hand:

• Modules where the side effects are mostly initialising module-internal state which is necessary for the correct operation of the functions/classes in the module's API should probably not be refactored, since in this case the exactly-once, before-use guarantees are desirable.

Modules with side effects to review

Items should be formatted * [ ] path/to/filename.js (Current.module.Name): followed by an explanatory note or link to existing discussion about the problem.

• blocks/*.js (All of them!): operate primarily by making calls to Blockly.defineBlocksWithJsonArray.
• core/contextmenu_items.js (Blockly.ContextMenuItems): calls own registerDefaultOptions function at startup which ends up calling ContextMenuRegistry.registry.register.
• …