refactor(blocks): Migrate `blocks/text.js` to TypeScript #6958

rachel-fenichel · 2023-04-07T20:18:41Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide
I ran npm run format and npm run lint

The details

Resolves

Part of #6828

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

rachel-fenichel · 2023-04-07T20:19:34Z

@cpcallen fyi I made a start on the text blocks, but will need type help to disentangle some of the combinations of extensions and mixins.

rachel-fenichel · 2023-04-19T23:29:15Z

blocks/text.ts

@@ -339,15 +334,15 @@ blocks['text_getSubstring'] = {
    const menu = fieldRegistry.fromJson({
      type: 'field_dropdown',
      options: this['WHERE_OPTIONS_' + n],


@cpcallen What do you think I should do for this dynamic access to WHERE_OPTIONS_1 and WHERE_OPTIONS_2?

First I would try typing the parameter n: 1|2, which should probably be the case in any case.

If that doesn't fix a type error here then I'd try this just in case it helps:

Suggested change

options: this['WHERE_OPTIONS_' + n],

options: this['WHERE_OPTIONS_' + (n as 1|2)],

but probably it wouldn't, so then it might be necessary to resort to :

Suggested change

options: this['WHERE_OPTIONS_' + n],

options: this[('WHERE_OPTIONS_' + n) as 'WHERE_OPTIONS_1'|'WHERE_OPTIONS_2'],

…and if that doesn't work then maybe ultimately it might be necessary to do something like:

Suggested change

options: this['WHERE_OPTIONS_' + n],

options: n === 1 ? this.WHERE_OPTIONS_1 : this.WHERE_OPTIONS_2;

(but I'd really rather we didn't!)

rachel-fenichel · 2023-04-19T23:31:00Z

blocks/text.ts

@@ -282,7 +279,7 @@ blocks['text_getSubstring'] = {
   * @return {!Element} XML storage element.
   * @this {Block}
   */
-  mutationToDom: function() {
+  mutationToDom: function(this: Block): Element {
    const container = xmlUtils.createElement('mutation');
    const isAt1 = this.getInput('AT1').type === ConnectionType.INPUT_VALUE;


@cpcallen this.getInput('AT') is possible null, because it's added and removed dynamically. Is there a way to guarantee that it's present, or should I just use ! to assert that it is non-null?

I think for now just use ! (since we presume the existing code is not broken), but then in the next pass we actually add checks for these things (via ?. where convenient and more explicitly via if (bad) throw new SomeError() where necessary).

rachel-fenichel · 2023-04-22T00:21:46Z

@cpcallen this is ready for review!

cpcallen

@cpcallen this is ready for review!

Don't forget to update the PR description.

cpcallen · 2023-04-24T19:21:53Z

blocks/text.ts

-    const isAt2 = this.getInput('AT2').type === ConnectionType.INPUT_VALUE;
-    container.setAttribute('at2', isAt2);
+    const isAt1 = this.getInput('AT1')!.type === ConnectionType.INPUT_VALUE;
+    container.setAttribute('at1', `${isAt1}`);


Note: I had a look recently to see if String(x) or `${x}` to typecast the second parameter of setAttribute calls. We use both, with String being slightly more popular.

blocks/text.ts

cpcallen · 2023-04-26T17:25:32Z

blocks/text.ts

   */
-  compose: function(containerBlock) {
-    let itemBlock = containerBlock.getInputTargetBlock('STACK');
+  compose: function(this: JoinMutatorBlock, containerBlock: Block) {


containerBlock could be a more specific type, if you define one.

cpcallen · 2023-04-26T17:26:29Z

blocks/text.ts

-  compose: function(containerBlock) {
-    let itemBlock = containerBlock.getInputTargetBlock('STACK');
+  compose: function(this: JoinMutatorBlock, containerBlock: Block) {
+    let itemBlock = containerBlock.getInputTargetBlock('STACK') as BlockSvg;


I think it would be better to say as JoinMutatorItemBlock here, to avoid the cast below.

If I do that, I have to cast every time I call getNextBlock, because it returns BlockSvg and that may not have a valueConnection_ on it.

I could declare it so that getNextBlock overrides and returns a JoinItemBlock but I don't think that's better than treating it as a BlockSvg overall and a JoinItemBlock when I need the valueConnection_.

(Attempt #N at writing a coherent reply comment here:)

I think casting the result of getNextBlock() is the correct thing to do in any case (at least pending adding an actual runtime type check in a future PR), but I don't think it matters very much for overall.

blocks/text.ts

cpcallen · 2023-04-26T17:27:09Z

blocks/text.ts

   */
-  saveConnections: function(containerBlock) {
+  saveConnections: function(this: JoinMutatorBlock, containerBlock: Block) {


Again: a more specific type for containerBlock should be possible and might be useful.

I'm neutral on the specific type for the container block. Do you want me to add it? My default is to leave it alone.

If it's not useful at the moment then I agree about leaving it alone.

cpcallen · 2023-04-26T17:30:27Z

blocks/text.ts

 */
-const TEXT_INDEXOF_TOOLTIP_EXTENSION = function() {
+const INDEXOF_TOOLTIP_EXTENSION = function(this: Block) {
  // Assign 'this' to a variable for use in the tooltip closure below.
  const thisBlock = this;
  this.setTooltip(function() {


Not done in previous blocks PRs, but noting the impending PR #7014 enables eslint in theblocks/ directory: might as well use an arrow function here, and avoid the wrath of eslint (and the need for const thisBlock = this).

cpcallen

With one nit (remove Mixin from a Block type's name) this LGTM.

cpcallen · 2023-04-28T15:26:14Z

blocks/text.ts

+/** Type of a block that has QUOTE_IMAGE_MIXIN */
+type QuoteImageMixinBlock = Block&QuoteImageMixin;
+interface QuoteImageMixin extends QuoteImageMixinType {}
+type QuoteImageMixinType = typeof QUOTE_IMAGE_MIXIN;


OK, but can we at least remove "Mixin" from QuoteImageMixinBlock? Because otherwise (for consistency with the naming convention established everywhere else) the other two should be QuoteImageMixinMixin and QuiteImageMixinMixinType.

cpcallen · 2023-04-28T15:36:52Z

blocks/text.ts

-  compose: function(containerBlock) {
-    let itemBlock = containerBlock.getInputTargetBlock('STACK');
+  compose: function(this: JoinMutatorBlock, containerBlock: Block) {
+    let itemBlock = containerBlock.getInputTargetBlock('STACK') as BlockSvg;


(Attempt #N at writing a coherent reply comment here:)

I think casting the result of getNextBlock() is the correct thing to do in any case (at least pending adding an actual runtime type check in a future PR), but I don't think it matters very much for overall.

rachel-fenichel added 2 commits April 7, 2023 11:21

refactor(blocks): auto-migration of blocks/text.js to ts

1e46f71

refactor(blocks): fix up some types in blocks/text.ts

be1df7b

rachel-fenichel commented Apr 19, 2023

View reviewed changes

cpcallen changed the title ~~Convert text~~ refactor(blocks): Migrate `blocks/text.js to TypeScript Apr 21, 2023

cpcallen changed the title ~~refactor(blocks): Migrate `blocks/text.js to TypeScript~~ refactor(blocks): Migrate blocks/text.js to TypeScript Apr 21, 2023

github-actions bot added PR: refactor and removed PR: refactor labels Apr 21, 2023

cpcallen mentioned this pull request Apr 21, 2023

Convert blocks/ and generators/ to TypeScript #6828

Closed

72 tasks

rachel-fenichel added 4 commits April 21, 2023 14:47

refactor(blocks): improve some types in text.ts

43f8b8f

refactor(blocks): additional type cleanups

5901930

chore: format

4f2da2f

chore: fix typo

5328dac

rachel-fenichel marked this pull request as ready for review April 22, 2023 00:21

rachel-fenichel requested a review from a team as a code owner April 22, 2023 00:21

rachel-fenichel requested a review from BeksOmega April 22, 2023 00:21

github-actions bot assigned BeksOmega Apr 22, 2023

BeksOmega assigned cpcallen and unassigned BeksOmega Apr 24, 2023

BeksOmega removed their request for review April 24, 2023 14:54

cpcallen requested changes Apr 26, 2023

View reviewed changes

chore: respond to PR comments

6079b7f

github-actions bot added PR: refactor and removed PR: refactor labels Apr 26, 2023

cpcallen approved these changes Apr 28, 2023

View reviewed changes

chore: respond to PR comments

31a4e2c

rachel-fenichel merged commit d47369a into google:develop Apr 28, 2023

rachel-fenichel deleted the convert_text branch April 28, 2023 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(blocks): Migrate `blocks/text.js` to TypeScript #6958

refactor(blocks): Migrate `blocks/text.js` to TypeScript #6958

rachel-fenichel commented Apr 7, 2023 •

edited

Loading

rachel-fenichel commented Apr 7, 2023

rachel-fenichel Apr 19, 2023 •

edited

Loading

cpcallen Apr 21, 2023

rachel-fenichel Apr 19, 2023

cpcallen Apr 21, 2023 •

edited

Loading

rachel-fenichel commented Apr 22, 2023

cpcallen left a comment

cpcallen Apr 24, 2023

cpcallen Apr 26, 2023

cpcallen Apr 26, 2023

rachel-fenichel Apr 26, 2023

cpcallen Apr 28, 2023

cpcallen Apr 26, 2023

rachel-fenichel Apr 26, 2023

cpcallen Apr 28, 2023

cpcallen Apr 26, 2023

cpcallen left a comment

cpcallen Apr 28, 2023

cpcallen Apr 28, 2023

	options: this['WHERE_OPTIONS_' + n],
	options: this['WHERE_OPTIONS_' + (n as 1\|2)],

	options: this['WHERE_OPTIONS_' + n],
	options: this[('WHERE_OPTIONS_' + n) as 'WHERE_OPTIONS_1'\|'WHERE_OPTIONS_2'],

	options: this['WHERE_OPTIONS_' + n],
	options: n === 1 ? this.WHERE_OPTIONS_1 : this.WHERE_OPTIONS_2;

refactor(blocks): Migrate blocks/text.js to TypeScript #6958

refactor(blocks): Migrate blocks/text.js to TypeScript #6958

Conversation

rachel-fenichel commented Apr 7, 2023 • edited Loading

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

rachel-fenichel commented Apr 7, 2023

rachel-fenichel Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcallen Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

rachel-fenichel commented Apr 22, 2023

cpcallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor(blocks): Migrate `blocks/text.js` to TypeScript #6958

refactor(blocks): Migrate `blocks/text.js` to TypeScript #6958

rachel-fenichel commented Apr 7, 2023 •

edited

Loading

rachel-fenichel Apr 19, 2023 •

edited

Loading

cpcallen Apr 21, 2023 •

edited

Loading