Review structure and style of goog.modules

[cpcallen]

Background

The first pass converting goog.provide to goog.module is for #5026 is intentionally being done quite mechanically so as to not change the API of any individual module and therefore to allow the work to proceed file-by-file.

In the process, there are going to be situations where it becomes clear that the name and/or structure of the exports of a module are inelegant, no longer well-structured or in violation of the style guide. Possible examples include:

1. Modules that export a single class constructor, where the class constructor is encumbered with a number properties whose values seem unrelated to the class. Examples:

   • A class MyClass where MyClass.CONST was some value that was not an instance of MyClass, or
   • …where MyClass.someFunction did not accept or return a value of type MyClass.

   Not examples—these are fine:

   • Classes with static methods (like Array.from on Array).
   • Enums with their corresponding value properties: MyEnum.value1, MyEnum.value2.

2. Modules named with a capital letter that do not have as their sole export the correspondingly-named class constructor. Examples:

   • A goog.module('….Name') that has exports = {id1, id2, id3}, or
   • …which has exports = Identifier where Identifier is not a class constructor.

[Add similar situations here.]

Fixing these issues naturally entails modifying the exported API of the module which consequently involves modifying the code which imports the module. We therefore wish to defer this work until the first pass, file-by-file conversion to goog.module is complete.

This bug is to track instances of the above issues so we can come back to them once the first pass is complete.

This bug is not intended to track cases where it is merely the contents of the API which is in need of review—i.e., where we think we wish to add items to (or remove them from) the exports list.

Items in need of further attention

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

Needing triage:

Add to this list any files which you discover which you think may need to be reviewed for inclusion in any of the other categories below. @cpcallen will review and tick completed items off.

• core/utils/coordinate.js (Blockly.utils.Coordinate): see Migrate core/utils/coordinate.js to goog.module syntax #5047.
• core/block.js (Blockly.Block): Does not require Blockly.Generator, but checks if it exists and then uses it. Unclear how to migrate to goog.require, since it's not required as such.
• core/bubble_dragger.js (Blockly.BubbleDragger): Uses WorkspaceCommentSvg in a type annotation without requiring it; adding a requireType for it breaks the build. Migrate core/bubble_dragger.js to goog.module syntax #5081
• core/constants.js (Blockly.constants): sets properties on Blockly with wild abandon; is often required with a suppress {extraRequire} as a result.
• core/registry.js (Blockly.registry): Type should probably be factored out into its own file.Migrate core/registry.js to goog.module syntax #5217
• core/events/events_selected.js (Blockly.Events.Selected): Selected would be much clearer as SelectedEvent. Migrate core/events/events_selected.js to goog.module syntax #5281
• events/events.js (Blockly.Events): Exports a bunch of constants that should probably be an enum. See Migrate core/events/events.js to goog.module syntax #5302.
• …

Exports a class constructor with unrelated static properties:

Where the static properties are a bunch of sequentially-numbered constants, consider whether they should be an enum instead, and then whether such an enum is most sensibly a static property of the constructor or whether it should be exported side-by-side with the constructor.

• core/connection.js (Blockly.Connection): Exports a bunch of constants that should probably be an enum. See Migrate core/connection.js to goog.module syntax #5102.
• core/utils/svg.js (Blockly.utils.Svg): Exports a bunch of Svg instances as static properties of the Svg constructor. See comment in refactor: Migrate to named exports #5623.

Named with a capital letter but does not export a class constructor:

• core/blockly.js (Blockly): not a class constructor (at least not yet). Users can import const Blockly = goog.require('blockly'); to avoid having to rename throughout existing codebases.
• core/extensions.js (Blockly.Extensions): noticed in Migrate core/extensions.js to goog.module syntax #5065.
• core/utils/idgenerator.js (Blockly.utils.IdGenerator).
• core/utils/keycodes.js (Blockly.utils.KeyCodes).
• core/blocks.js (Blockly.Blocks).
• …

Includes extra requires that are suppressed with /** @suppress {extraRequire} */

• core/connection.js
• core/renderers/common/info.js #5596
• …

Circular dependencies

• core/gesture.js and core/blockly.js
• core/variable_map.js and core/blockly.js
• core/scrollbar.js and core/blockly.js
• core/workspace_dragger.js and core/blockly.js
• core/insertion_marker_manager.js and core/blockly.js
• core/contextmenu.js and core/blockly.js
• core/names.js and core/procedures.js
• core/workspace.js and core/block.js
• …

Exports a singleton and the singleton's constructor

Where a module provides a singleton object—and we do not want or expect there to be any other instances of the singleton's class—only the singleton should be exported. (Need to investigate how to properly export the type of a singleton without exporting a callable constructor.)

• core/shortcut_registry.js (Blockly.ShortcutRegistry): only export singleton, and probably pick better name for the export (see Migrate core/shortcut_registry.js to goog.module syntax #5219).
• …

Module namespace does not match directory structure

From the style guide section 3.3.1:
"The directory hierarchy reflects the namespace hierarchy, so that deeper-nested children are subdirectories of higher-level parent directories... For example, the goog namespace is for the Closure Library, and other engineers expect to find goog.foo.bar at //google3/third_party/javascript/closure/foo/bar.js."

• core/renderers/common/*.js uses namepace Blockly.blockRendering.*, but would be more readable if it did follow this rule and had the namespace Blockly.renderers.common.*
• …