feat!: add block definitions to colour field plugin

[rachel-fenichel]

The basics

• I validated my changes

The details

Resolves

Fixes #2152

Proposed Changes

• Adds block definitions and generators for four blocks associated with the colour field:
  • colour_blend
  • colour_rgb
  • colour_picker
  • colour_random
• Demonstrates a pattern for defining and exporting a library of blocks.

BREAKING CHANGE: The colour field no longer registers itself on load. The developer must either manually register the field or install blocks, which will install the field. This is part of a move to have no side effects in field and block definitions, so that tree-shaking can remove unwanted fields and blocks.

Reason for Changes

This is the next step in extracting block definitions from the core library and publishing them as smaller libraries. Developers can then choose which blocks and fields to install, and can tree-shake out unneeded ones.

Test Coverage

• Updated mocha tests to call registerColourField to unregister previous field (from core).
• Updated test playground to delete/unregister blocks, generators, and the field from core, then install/register the new blocks, generators, and field.
• Stripped use of old generateFieldTestBlocks helper in favor of manually defining test blocks and the test toolbox.
  • I found the result easier to read and modify as I tried to test things, and it won't change frequently.

The test toolbox looks like this:

The first part of the toolbox tests the exported blocks. The later sections test the implementation of the colour field.

Documentation

This PR includes documentation updates in the README.

[cpcallen]

I've had a look at the first one, and it mostly looks good but I do have some… opinions.

[cpcallen]

I'd be tempted to put the reserved words in a property on the generator function and let installGenerators retrieve them from there, but I know that some of my colleagues have this funny idea that functions shouldn't be treated like normal objects…

I observe that doing that would make this install function less useful as a guide to how to install the required individual pieces—though in fact the installGenerators function already does that to large extent.

I think I might favour doing it inline:

  if(generators.dart) generators.dart.forBlock[blockName] = jsGenerator;
  if(generators.lua) generators.lua.forBlock[blockName] = luaGenerator;
  //..

(With judicious choice of variable names it may be possible to keep each of these on a single line.)

[rachel-fenichel]

If I do it inline for colour_picker, it looks like this:

export function installBlock(generators: Generators = {}) {
    registerColourField();
    Blockly.common.defineBlocks([blockDef]);
    if (generators.javascript) generators.javascript.forBlock[BLOCK_NAME] = jsGenerator;
    if (generators.dart) generators.dart.forBlock[BLOCK_NAME] = dartGenerator;
    if (generators.lua) generators.lua.forBlock[BLOCK_NAME] = luaGenerator;
    if (generators.php) generators.php.forBlock[BLOCK_NAME] = phpGenerator;
    if (generators.python) generators.python.forBlock[BLOCK_NAME] = pythonGenerator;
}

And if I extend that to a case where I do have reserved words, like colour_blend, I get this:

export function installBlock(generators: Generators = {}) {
    registerColourField();
    Blockly.common.defineBlocks([blockDef]);
    if (generators.javascript) generators.javascript.forBlock[BLOCK_NAME] = jsGenerator;
    if (generators.dart) {
        generators.dart.forBlock[BLOCK_NAME] = dartGenerator;
        generators.dart.addReservedWords('Math');
    }
    if (generators.lua) generators.lua.forBlock[BLOCK_NAME] = luaGenerator;
    if (generators.php) generators.php.forBlock[BLOCK_NAME] = phpGenerator;
    if (generators.python) generators.python.forBlock[BLOCK_NAME] = pythonGenerator;
}

It's verbose, but has several advantages:

• Keeps reserved words and block generator installation together.
• The installBlock function actually shows everything it's doing
  • As you observed, installGenerators was obfuscating.

The disadvantages are:

• Lots of text to copy.
• Lots of duplicated code.

I think this is worth doing, even if it adds duplicated code. The types did help me avoid installing a dart block generator into python, and that makes me happy. I think it's worth the change. @maribethb do you have any thoughts or objections?

[rachel-fenichel]

If I do it inline for colour_picker, it looks like this:

export function installBlock(generators: Generators = {}) {
    registerColourField();
    Blockly.common.defineBlocks([blockDef]);
    if (generators.javascript) generators.javascript.forBlock[BLOCK_NAME] = jsGenerator;
    if (generators.dart) generators.dart.forBlock[BLOCK_NAME] = dartGenerator;
    if (generators.lua) generators.lua.forBlock[BLOCK_NAME] = luaGenerator;
    if (generators.php) generators.php.forBlock[BLOCK_NAME] = phpGenerator;
    if (generators.python) generators.python.forBlock[BLOCK_NAME] = pythonGenerator;
}

And if I extend that to a case where I do have reserved words, like colour_blend, I get this:

export function installBlock(generators: Generators = {}) {
    registerColourField();
    Blockly.common.defineBlocks([blockDef]);
    if (generators.javascript) generators.javascript.forBlock[BLOCK_NAME] = jsGenerator;
    if (generators.dart) {
        generators.dart.forBlock[BLOCK_NAME] = dartGenerator;
        generators.dart.addReservedWords('Math');
    }
    if (generators.lua) generators.lua.forBlock[BLOCK_NAME] = luaGenerator;
    if (generators.php) generators.php.forBlock[BLOCK_NAME] = phpGenerator;
    if (generators.python) generators.python.forBlock[BLOCK_NAME] = pythonGenerator;
}

It's verbose, but has several advantages:

• Keeps reserved words and block generator installation together.
• The installBlock function actually shows everything it's doing
  • As you observed, installGenerators was obfuscating.

The disadvantages are:

• Lots of text to copy.
• Lots of duplicated code.

I think this is worth doing, even if it adds duplicated code. The types did help me avoid installing a dart block generator into python, and that makes me happy. I think it's worth the change. @maribethb do you have any thoughts or objections?

[rachel-fenichel]

I have updated this PR to only change the colour field.

[rachel-fenichel]

Open question: where should I document the generators parameter in more detail? Where should the type live? Currently it's in blocks/generatorUtils.ts and needs to be duplicated into every plugin where I define blocks.

[cpcallen]

Hmm. Do we auto-generate API documentation for plugins like we do for core?

In any case it seems like the best approach is probably to declare and explain the type in GeneratorUtils.ts (possibly renaming this file, if it no longer contains any functions), but have the individual install functions point to some examples in the README rather than trying to explain in detail (and repeatedly) exactly how this parameter works.

[rachel-fenichel]

My concern is that I will also need to declare this type in field_angle and field_multilinetextinput when I update those, because there's no shared place for this type to live.

[cpcallen]

Are you proposing to move the declaration to Blockly itself? I can see an argument in favour (it would be useful to have in a common location, and why not add it to the thing that all the client code will be importing anyway?), but on balance I am against adding a type to core that's not used anywhere in core.

There might be an argument for adding it to a utility package of field and block definition support code, but so far I think it's just this one type, and the type is straightforward enough that I don't think it's worth creating a separate package for it. DRY is a useful principle but not IMHO a compelling reason to incur the expense of an extra dependency for this.

If we end up with a bunch of declarations that are actually complicated enough to be worth factoring out then I would think the utility package would be the way to go.

Of course your original question pertains to documenting the type. To that I feel like the examples usages in the readme are sufficient, especially in conjunction with the type's declaration and JSDoc comment.

[rachel-fenichel]

This pull request is now (finally!) ready for review. Thanks for putting up with all the back and forth on this one.

[cpcallen]

Generally looking good, but still a few nits and some dissatisfaction with naming.

• The local variable name suggestions are from my perspective just that, though I think mostly in line with the styleguide's rules.
• The export naming commens are about making sure we will be happy with these names in future / in other similar packages.

Also, one major item: there appear to be unittests for the colour block generators in core, and these should be reimplemented in samples. I'd be fine with this being done in a separate PR, but I think they should be done (and passing) before this PR gets released.

[cpcallen]

This plugin should work with -beta.1 as well as the forthcoming v10 release, no?

[cpcallen]

Maybe generator_types.ts or just generators.ts?

[cpcallen]

Per practice in the core repo, I think imports of files from the same package should use the full (compiled) filename:

Suggested change

	import {registerFieldColour} from '../field_colour';
	import {Generators} from './generatorUtils';
	import {registerFieldColour} from '../field_colour.js';
	import {Generators} from './generatorUtils.js';

This doesn't matter so long as we are webpacking the file, but will matter if we start publishing unbundled packages (which we are intending to).

(Here and most files below.0

[rachel-fenichel]

Changing this to use .js extensions failed with this error:

ERROR in ./src/blocks/colourBlend.ts 12:0-57
Module not found: Error: Can't resolve '../field_colour.js' in '/home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/blocks'
resolve '../field_colour.js' in '/home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/blocks'
  using description file: /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/package.json (relative path: ./src/blocks)
    Field 'browser' doesn't contain a valid alias configuration
    using description file: /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/package.json (relative path: ./src/field_colour.js)
      no extension
        Field 'browser' doesn't contain a valid alias configuration
        /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/field_colour.js doesn't exist
      .ts
        Field 'browser' doesn't contain a valid alias configuration
        /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/field_colour.js.ts doesn't exist
      .js
        Field 'browser' doesn't contain a valid alias configuration
        /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/field_colour.js.js doesn't exist
      as directory
        /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/field_colour.js doesn't exist
 @ ./src/index.ts 10:0-52 17:0-54 18:0-40 30:4-28
 @ ./test/index.ts 16:0-71 272:4-23

[rachel-fenichel]

I've addressed (or punted) all comments except for the file extensions, which I'm still digging into.

After discussion with Christopher, I'm going to make a blocks_for_fields branch off master that I can merge successive field changes against, so that I can make smaller PRs to fix the follow-ups without accidentally releasing anything.

[rachel-fenichel]

(Extra comment below that I forgot to include in the previous review).

[rachel-fenichel]

Changing this to use .js extensions failed with this error:

ERROR in ./src/blocks/colourBlend.ts 12:0-57
Module not found: Error: Can't resolve '../field_colour.js' in '/home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/blocks'
resolve '../field_colour.js' in '/home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/blocks'
  using description file: /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/package.json (relative path: ./src/blocks)
    Field 'browser' doesn't contain a valid alias configuration
    using description file: /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/package.json (relative path: ./src/field_colour.js)
      no extension
        Field 'browser' doesn't contain a valid alias configuration
        /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/field_colour.js doesn't exist
      .ts
        Field 'browser' doesn't contain a valid alias configuration
        /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/field_colour.js.ts doesn't exist
      .js
        Field 'browser' doesn't contain a valid alias configuration
        /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/field_colour.js.js doesn't exist
      as directory
        /home/fenichel/Documents/blockly-dev/blockly-samples/plugins/field-colour/src/field_colour.js doesn't exist
 @ ./src/index.ts 10:0-52 17:0-54 18:0-40 30:4-28
 @ ./test/index.ts 16:0-71 272:4-23

[cpcallen]

Approved after a very cursory re-review.