feat: add extension point for overriding Grid functionality

[riknoll]

The basics

• I validated my changes

The details

Resolves

Fixes #8614

Proposed Changes

Refactors the Grid class so that it can be overridden by a plugin. The static methods for parsing grid options and creating the SVG pattern element have also been moved onto a new GridProvider class which can be passed in as a plugin in the BlocklyOptions object. The implementations of these functions are unchanged; they just got moved around.

Originally, I just wanted these to be static methods on the IGrid interface, but the typings for the registry's getClass functions don't really allow for static properties and I didn't want to tear all that up in this PR.

I would love some feedback on the typing of GridOptions here. My example use case for this is the MakeCode Minecraft editor which overrides the grid with a background image that scrolls with the workspace. A plugin that provided that functionality would likely need to extend the GridOptions type with extra fields like the URL of the image, width/height, opacity, etc. However, the grid property of the BlocklyOptions object passed to inject is pretty strongly typed; the only way around it right now is to do a lot of casting to any.

Reason for Changes

See the linked issue for reasoning!

Test Coverage

Tested manually in the playground and by creating a test plugin locally. Happy to add unit tests, but I didn't see any for other extension points at a click glance.

Documentation

Additional Information

I can also add a plugin to blockly-samples that utilizes this change should this PR be approved.

[rachel-fenichel]

@gonfunko bump

[gonfunko]

Thank you for this change, and apologies for taking a while on the review! Broadly speaking making the grid swappable seems great, but I think there are some structural issues we might want to address while we're creating a new public API. In particular, I think the way things currently work in core is a bit silly, and the existing indirection complicates making this pluggable. However, I think the implementation can be adjusted to make the API cleaner without too much trouble.

As things stand, the flow is:

1. Grid options can be specified during the call to inject()
2. inject() calls createDom() (in inject.ts), which calls Grid.createDom() and stores the resulting SVG pattern on the options object
3. A WorkspaceSvg is constructed, which creates a new Grid object from the grid options specified in the call to inject() and the SVG pattern created and stashed on the options object in step 2

Instead, I think we want to:

1. Continue to specify grid options during the call to inject, and have those parsed out in options.ts as they currently are
2. Do nothing grid-related in the createDom() in inject.ts.
3. Have WorkspaceSvg continue to initialize a Grid in its constructor, but (a) get the class from the registry and (b) pass no arguments to the Grid constructor
4. In WorkspaceSvg.createDom():

• Call this.grid.createDom(), which is now an instance method, passing only the parsed grid options
• Assign a random ID to the element returned from that method
• Create a element, add the returned element as a child, and add the to the injection div (this currently happens in inject.ts)

This has the following benefits:

• Removes the need for Options.gridPattern, which is the only field there that isn't really a configuration option as such
• Removes the need for the rnd, defs, and (in v12) injectionDiv arguments to Grid.createDom(), which are quite low-level and require the implementation to follow a lot of contracts. Instead, implementers need only concern themselves with creating and returning an SVG pattern, and Blockly will handle its own internal bookkeeping.
• Makes Grid.createDom() an instance method, which works easier with interfaces as you noted
• Eliminates the need for IGridProvider
• Allows developers to add their own configuration methods to their implementations of IGrid, which they can use to configure properties, e.g. the image URL, size, opacity etc. that you noted. IMO this is strongly preferable to tacking arbitrary properties on to the grid injection option; that has the typing problems you described, and broadly speaking we don't really want to add options there in general. With this approach, after injection, developers can just call workspace.getGrid().setWhateverConfigOption(); those methods won't be part of the IGrid interface, but since they just injected their own version they can confidently cast the return type of getGrid() to their concrete implementation.

[gonfunko]

Suggest removing this; it's quite low level, and its only caller in core is in WorkspaceSvg, which can be eliminated easily if we follow the approach I outlined in my top-level comment.

[gonfunko]

Let's make this setScale() and getScale() for consistency. The built-in Grid implementation can still call update() to adjust itself in response to changes in its configuration, but that doesn't need to be part of the interface (or require scale as an argument).

[gonfunko]

Throughout, I don't think we want @internal annotations in the interface; if we're committing it to the interface, that makes it public API.