blockly-samples icon indicating copy to clipboard operation
blockly-samples copied to clipboard

chore: convert field-slider plugin to TypeScript

Open btw17 opened this issue 3 years ago • 6 comments

Description

  • Updated plugins/field-slider to use TypeScript for its src code and demo test code.
  • Updated plugins/dev-tools to require Blockly > 8.0.3 to be compatible.

Note

  • The blockly.d.ts export in blockly core is not compatible with this package prior to 8.0.3.
  • The test/*.mocha.js files are left unchanged since the dev scripts do not support TS currently.
  • The .d.ts files for plugins/field-slider will not be generated due to an issue with plugins/dev-scripts. A fix was found for that issue, though it won't be accepted until the 'core Blockly to TS' is resolved since there is some crossover in the fix's implementation.

Discussion

  • There are some TODO notes related to type information for core Blockly packages, though there's a large TS conversion in that incoming package which will likely impact the type information. Should:
    • Individual issues be created for each TODO?
    • One group issue be created for all of those TODOs?
    • Those TODOs be attributed to the incoming TS conversion?
    • The TODOs be removed for now?

btw17 avatar Aug 05 '22 16:08 btw17

I'm assigning @maribethb as well, because this needs her opinions on bumping the dev-tools dependency. Bumping the min version of Blockly requires a major bump for the dev-tools version, and I just want to double check we're cool with doing that.

BeksOmega avatar Aug 05 '22 16:08 BeksOmega

The build issue is an interesting one. When running a clean install from plugins/field-slider, the build works fine. When running a clean install from the root, though, there's an issue with the blockly package for building. (The resulting plugins/field-slider/package-lock.json also varies based on where the install is from.) My guess is it's related to how lerna is linking the packages, though I'll keep investigating.

btw17 avatar Aug 05 '22 16:08 btw17

Can you insert a commit that changes the file extensions .js to .ts without actually modifying the contents of the files? That way we can ensure that git understands we're just renaming files, not deleting them and creating new ones, which is important for maintaining our git blame.

Done!

btw17 avatar Aug 05 '22 16:08 btw17

This will need the change in #1299 as well.

rachel-fenichel avatar Sep 21 '22 22:09 rachel-fenichel

Looks like blockly-scripts doesn't work with the override keyword. I removed it from render_ and showEditor_, though I'd originally added it to those since an override comment was present.

In TypeScript, override ensures that the method we're trying to override exists in the parent. This is pretty handy if, for example, the render_() method were updated to renderInternal() or something similar since the compiler would catch it. I'm not sure if this is a blocking issue, though. (See the TypeScript 4.3 documentation for more about override.)

It's also probably worth investigating why blockly-scripts doesn't work with override.

btw17 avatar Sep 23 '22 21:09 btw17

For the failing tests, it looks like the updated Blockly imports break outside of my changes.

For example, going to blockly-samples/plugins/block-plus-minus/test/if.mocha.js, Blockly.Dart does not exist.

btw17 avatar Sep 23 '22 21:09 btw17