refactor(generators): Migrate JavaScript generators to TypeScript #7602

cpcallen · 2023-10-20T16:33:08Z

The basics

I validated my changes

The details

Resolves

Part of #6828.

Proposed Changes

Migrate generators/javascript/*.js and generators/javascript.js to TypeScript.

Reason for Changes

Finish TS migration.

Test Coverage

npm test passes. No changes to manual test procedures required.

Documentation

No changes to documentation yet, as there should be minimal changes to the generated code and no changes to the published typings (because during build we overwrite the generated .d.ts files with the ones from typings/).

Additional Information

It may be easier to review this commit-by-commit, as the final format commit make a lot of noisy changes.

Slightly simplify the implementation of getAdjusted, in part to make it more readable. Also improve its JSDoc comment.

First pass doing very mechanistic migration, not attempting to fix all the resulting type errors.

This consists almost entirely of adding casts, so the code output by tsc should be as similar as possible to the pre-migration .js source files.

The way the generator functions are added to javascriptGenerator.forBlock has been modified so that incorrect generator function signatures will cause tsc to generate a type error.

One block protected with // prettier-ignore to preserve careful comment formatting. Where there are repeated concatenations prettier has made a pretty mess of things, but the correct fix is probably to use template literals instead (rather than just locally disabling prettier). This has been added to the to-do list in google#7600.

generators/javascript/javascript_generator.ts

BeksOmega · 2023-10-20T17:42:08Z

generators/javascript/javascript_generator.ts

-  getAdjusted(block, atId, opt_delta, opt_negate, opt_order) {
-    let delta = opt_delta || 0;
-    let order = opt_order || this.ORDER_NONE;
+  getAdjusted(block: Block, atId: string, delta = 0, negate = false, order = Order.NONE): string|number {


Switching order from number= to = Order.NONE is a safe change right?

Oh. Hmm. This is sort of notionally a breaking change, but:

(x: number) => any and (x = Order.NONE) => any are mutually assignable (even though it's unsound to pass a non-Order number to the latter function).

Any code that passes in a non-Order number is probably (but not certainly) wrong.

Part of the purpose of the TS migration is to break wrong code.

Any breakage (outside of the core repo) will not actually be seen until we start publishing types; at the moment javascriptGenerator is typed as any.

So overall I think this is fine—but I nevertheless appreciate being made to go and actually check the effects of the narrowed type.

BeksOmega · 2023-10-20T17:43:30Z

generators/javascript/javascript_generator.ts

    }

-    let at = this.valueToCode(block, atId, outerOrder) || defaultAtIndex;
+    let at = this.valueToCode(block, atId, innerOrder) || defaultAtIndex;


I think this naming is confusing. You should never pass an innerOrder to valueToCode, because valueToCode expects the strongest order acting on the child block.

Note to self: review getAdjusted more thoroughly once naming is changed.

I've changed the name to orderForInput.

BeksOmega · 2023-10-20T17:50:19Z

generators/javascript/logic.ts

 import {Order} from './javascript_generator.js';


-export function controls_if(block, generator) {
+export function controls_if(block: Block, generator: JavascriptGenerator) {


It looks like statement blocks are missing return types, is that intentional?

Expression blocks require an explicit return type of [string, Order] as the return type is otherwise inferred to be (string | Order)[], which causes all kinds of type errors in the places where the return values are consumed.

For statements, the return type is correctly inferred from the return statement (either a string or null). Do you think it worthwhile to specify the return type explicitly? If so, do you think it preferable to specify it as either string or null, or as string | null?

Or, should they all be string | null | [string, Order]?

blocks/loops.ts

BeksOmega · 2023-10-20T17:55:49Z

generators/javascript/lists.ts

@@ -268,11 +270,19 @@ const getSubstringIndex = function(listName: string, where: string, opt_at?: str
 };

 export function lists_getSublist(block: Block, generator: JavascriptGenerator): [string, Order] {
+  // Dictionary of WHEREn field choices and their CamelCase equivalents.


nit: WHERE field choices and their PascalCase equiavlents, or just delete comment.

The n is not a typo. There is no field called WHERE, but there are WHERE1, WHERE2 etc.

generators/javascript/procedures.ts

BeksOmega · 2023-10-20T18:06:38Z

generators/javascript/javascript_generator.ts

+      // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#Keywords
+      'break,case,catch,class,const,continue,debugger,default,delete,do,' +
        'else,export,extends,finally,for,function,if,import,in,instanceof,' +


This is a weird thing it did. Might want to report this as a prettier bug. Or just move the comment to outside the parameter list (above this.addReservedWords instead).

I moved the comment since I thought that worth doing anyway, but it did not result in prettier formatting the argument any differently, and indeed I think it is actually doing the right thing here: there is a single argument to addReservedWords, and that argument is in fact split over multiple lines, and the normal rule is that continuation lines should be indented (+4 in Google style, +2 in Prettier style).

cpcallen · 2023-10-25T21:31:09Z

I believe this is ready for re-review.

BeksOmega

Fix the build error then LGTM!

cpcallen · 2023-10-30T11:46:37Z

Fix the build error then LGTM!

🥚U+200D😳

…7617) * refactor(generators): Migrate python_generator.js to TypeScript * refactor(generators): Migrate generators/python/* to TypeScript First pass doing very mechanistic migration, not attempting to fix all the resulting type errors. * fix(generators): Fix type errors in generator functions This consists almost entirely of adding casts, so the code output by tsc should be as similar as possible to the pre-migration .js source files. * refactor(generators): Migrate generators/python.js to TypeScript The way the generator functions are added to pythonGenerator.forBlock has been modified so that incorrect generator function signatures will cause tsc to generate a type error. * chore(generator): Format One block protected with // prettier-ignore to preserve careful comment formatting. Where there are repeated concatenations prettier has made a pretty mess of things, but the correct fix is probably to use template literals instead (rather than just locally disabling prettier). This is one of the items in the to-do list in #7600.

@Protected

In PRs google#7602, google#7616, google#7646, google#7647 and google#7654 the @Protected access modifier on scrub_ was not transcribed to the new typescript signature. BREAKING CHANGE: This makes scrub_ protected again, which will break any code attempting to access this method from outside a CodeGenerator subclass.

@Protected

Fixes google#2156. In PRs google#7602, google#7616, google#7646, google#7647 and google#7654 the @Protected access modifier on scrub_ on the CodeGenerator subclasses was not transcribed to the new typescript signature. I was going to re-add it, but this breaks some of the procedure block generator functions which rely on it, and then @BeksOmega pointed out that this might be one of the CodeGenerator API functions which we had already decided should be public—and lo and behold I found google#2156. Per discussion amongst team, I am not renaming it to scrub at this time.

@Protected

Fixes google#2156. In PRs google#7602, google#7616, google#7646, google#7647 and google#7654 the @Protected access modifier on scrub_ on the CodeGenerator subclasses was not transcribed to the new typescript signature. I was going to re-add it, but this breaks some of the procedure block generator functions which rely on it, and then @BeksOmega pointed out that this might be one of the CodeGenerator API functions which we had already decided should be public—and lo and behold I found google#2156. Per discussion amongst team, I am not renaming it to scrub at this time.

@Protected

Fixes google#2156. In PRs google#7602, google#7616, google#7646, google#7647 and google#7654 the @Protected access modifier on scrub_ on the CodeGenerator subclasses was not transcribed to the new typescript signature. I was going to re-add it, but this breaks some of the procedure block generator functions which rely on it, and then @BeksOmega pointed out that this might be one of the CodeGenerator API functions which we had already decided should be public—and lo and behold I found google#2156. Per discussion amongst team, I am not renaming it to scrub at this time.

@Protected

Fixes #2156. In PRs #7602, #7616, #7646, #7647 and #7654 the @Protected access modifier on scrub_ on the CodeGenerator subclasses was not transcribed to the new typescript signature. I was going to re-add it, but this breaks some of the procedure block generator functions which rely on it, and then @BeksOmega pointed out that this might be one of the CodeGenerator API functions which we had already decided should be public—and lo and behold I found #2156. Per discussion amongst team, I am not renaming it to scrub at this time.

cpcallen added 6 commits October 20, 2023 17:25

refactor(generators): Migrate javascript_generator.js to TypeScript

cf5a2f9

refactor(generators): Simplify getAdjusted

e8dfca9

Slightly simplify the implementation of getAdjusted, in part to make it more readable. Also improve its JSDoc comment.

refactor(generators): Migrate generators/javascript/* to TypeScript

d15e802

First pass doing very mechanistic migration, not attempting to fix all the resulting type errors.

fix(generators): Fix type errors in generator functions

3df3247

This consists almost entirely of adding casts, so the code output by tsc should be as similar as possible to the pre-migration .js source files.

refactor(generators): Migrate generators/javascript.js to TypeScript

a93dc78

The way the generator functions are added to javascriptGenerator.forBlock has been modified so that incorrect generator function signatures will cause tsc to generate a type error.

cpcallen added component: generators PR: refactor labels Oct 20, 2023

cpcallen requested a review from a team as a code owner October 20, 2023 16:33

cpcallen requested a review from BeksOmega October 20, 2023 16:33

github-actions bot assigned BeksOmega Oct 20, 2023

github-actions bot added PR: refactor and removed PR: refactor labels Oct 20, 2023

BeksOmega requested changes Oct 20, 2023

View reviewed changes

fix(generators): Fixes for PR google#7602

992987c

BeksOmega approved these changes Oct 25, 2023

View reviewed changes

fix(generators): Fix syntax error

b1bf0ea

cpcallen merged commit 4ab8d00 into google:develop Oct 30, 2023
7 checks passed

cpcallen deleted the refactor/6828/javascript branch October 30, 2023 08:16

cpcallen mentioned this pull request Mar 15, 2024

fix(generators): Make scrub_ public #7940

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(generators): Migrate JavaScript generators to TypeScript #7602

refactor(generators): Migrate JavaScript generators to TypeScript #7602

cpcallen commented Oct 20, 2023

BeksOmega Oct 20, 2023

cpcallen Oct 25, 2023

BeksOmega Oct 20, 2023 •

edited

Loading

cpcallen Oct 25, 2023

BeksOmega Oct 20, 2023

cpcallen Oct 25, 2023

BeksOmega Oct 20, 2023

cpcallen Oct 25, 2023

BeksOmega Oct 20, 2023

cpcallen Oct 25, 2023

cpcallen commented Oct 25, 2023

BeksOmega left a comment

cpcallen commented Oct 30, 2023

refactor(generators): Migrate JavaScript generators to TypeScript #7602

refactor(generators): Migrate JavaScript generators to TypeScript #7602

Conversation

cpcallen commented Oct 20, 2023

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeksOmega Oct 20, 2023 • edited Loading

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 commented Oct 25, 2023

BeksOmega left a comment

Choose a reason for hiding this comment

cpcallen commented Oct 30, 2023

BeksOmega Oct 20, 2023 •

edited

Loading