feat: Add a JSON Block definition interface#9613
feat: Add a JSON Block definition interface#9613heliacer wants to merge 1 commit intoRaspberryPiFoundation:mainfrom
Conversation
|
hi, I noticed the review assignment changed, |
|
bump again :D |
maribethb
left a comment
maribethb
left a comment
There was a problem hiding this comment.
Thank you so much for your patience while we reviewed this. This needed some consultation from our resident TS expert and after chatting with him I think we want to go in a different direction for custom fields for now. But the rest of this is mostly solid, thank you for your work on it.
| { | ||
| 'type': 'field_variable', | ||
| 'name': 'VAR', | ||
| 'variable': null, |
There was a problem hiding this comment.
The change to this file should not be made. You are changing the block definitions in unexpected ways.
| 'variable': '%{BKY_VARIABLES_DEFAULT_NAME}', | ||
| }, | ||
| ], | ||
| 'output': null, |
There was a problem hiding this comment.
The change to this file should also be reverted. You've removed an output from one block and added it to a different one unexpectedly. This will change the function and shape of the blocks.
| * | ||
| * @example | ||
| * ```typescript | ||
| * declare module 'blockly/core/interfaces/i_json_block_definition' { |
There was a problem hiding this comment.
This is an interesting idea, but I don't think it's a pattern we want to add here. Using declare module to augment the types might work, but we'd probably want to implement it in a different way so that developers don't have to use this specific filepath which is more of an implementation detail.
I think for now, it would be best to have custom fields/inputs/args use unknown type. While this isn't as helpful as having a fully typed interface, it would let developers create their own types for their own internal use but pass them to Blockly as unknown, which is an acceptable compromise at this point.
| name?: string; | ||
| } | ||
|
|
||
| export type FieldsAlign = 'LEFT' | 'RIGHT' | 'CENTRE'; |
There was a problem hiding this comment.
We might want to use Blockly.inputs.Align for this instead of redeclaring these strings.
|
|
||
| import './different_user_input'; | ||
|
|
||
| declare module 'blockly-test/core/interfaces/i_json_block_definition' { |
There was a problem hiding this comment.
As discussed above, if you have the internal Blockly types use unknown for custom fields, you should still be able to declare and use this type locally (so without the declare module bit).
This is a follow-up of #9402 :D (please read)
Changes:
tests/typescriptfor field mitosisThe custom fields are done by augmenting, as shown in the jsdoc example.
Also I'm not too proud of the documentation strings in general but I'm leaving it as-is now.
lmk if I need to change something in order to merge :>