Convert nodes.js to TypeScript #1748

kevinb-khan · 2024-10-13T18:58:45Z

Summary:

After updating nodes.js to ues ES6 classes in #1747, the move to TypeScript became much more feasible. While this PR mostly just adds types in a bunch of places, there are some changes to the code itself in order to minimize the number of casts and @ts-expect-errors that were require. I've tried to keep these kinds of changes to a minimum.

I ended up converting a bunch of function expressions to arrow functions. Hopefully that doesn't add too much additional overhead when reviewing. There's still more cleanup that could be done (adding more missing types, converting all vars to either const or let, etc.), but I'm going to leave that for a future PR.

Issue: None

Test plan:

yarn test packages/kas/src

khan-actions-bot · 2024-10-13T18:58:59Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/rare-pumas-rule.md, packages/kas/src/nodes.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2024-10-13T19:03:01Z

Size Change: +841 B (+0.1%)

Total Size: 867 kB

Filename	Size	Change
`packages/kas/dist/es/index.js`	38.8 kB	+841 B (+2.21%)

ℹ️ View Unchanged

Filename	Size
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	78 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	280 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/index.js`	420 kB
`packages/perseus/dist/es/strings.js`	3.4 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

kevinb-khan · 2024-10-13T19:02:08Z

packages/kas/src/nodes.ts


        return Mul.handleDivide(a, b).simplify();
    }
 }

 /* abstract symbol node */
-class Symbol extends Expr {
+class Sym extends Expr {


I renamed this to Sym so that I could use Symbol for the prefixes types down below.

Even though we could do something other than using Symbol for the prefix types, I'd like avoid shadowing built-ins like this.

packages/kas/src/nodes.ts

kevinb-khan · 2024-10-13T19:16:33Z

packages/kas/src/nodes.ts

+const NumNeg = new Int(-1).addHint("negate");
+const NumSub = new Int(-1).addHint("subtract");
+const NumDiv = new Int(-1).addHint("divide");

-Num.Sqrt = new Rational(1, 2).addHint("root");
+const NumSqrt = new Rational(1, 2).addHint("root");

-Num.Zero = new Int(0);
-Num.One = new Int(1);
-Num.Ten = new Int(10);
-
-// set identities here
-Add.prototype.identity = Num.Zero;
-Mul.prototype.identity = Num.One;
+const NumZero = new Int(0);
+const NumOne = new Int(1);
+const NumTen = new Int(10);


This can't be added to the Num class as static properties because Int and Rational depend on Num being defined. We could probably drop the Num prefix. Then we could export Zero and One directly here instead of down below.

github-actions · 2024-10-13T21:06:06Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (00596dd) and published it to npm. You
can install it using the tag PR1748.

Example:

yarn add @khanacademy/perseus@PR1748

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1748

handeyeco

I'm going to have to circle back to this tomorrow. Just some initial thoughts for now.

handeyeco · 2024-10-15T21:21:34Z

packages/kas/src/nodes.ts

+    return [a, b];
+}
+
+function isExpr(arg: string | Expr): arg is Expr {


Not a blocker, just a thought: would it be worth having these as like static methods? Like Expr.isInstance? Then we could make it abstract and force children to implement it; that way each variation would have a builtin predicate? idk, maybe more trouble than its worth, I just noticed the expr instanceof Mul below which made me think of it.

I'm not sure if type predicates can be generic. I'll have to experiment with that.

packages/kas/src/nodes.ts

handeyeco · 2024-10-15T21:30:30Z

packages/kas/src/nodes.ts

@@ -98,25 +134,43 @@ class Expr {
    }

    // an abstraction for chainable, bottom-up recursion
-    recurse(method, ...passed) {
+    recurse(method: string, ...passed: any[]): this {


I see a lot of anys, is that because we don't understand the types or because inheritance dictates what types will be used by different implementations?

This method is highly dynamic which makes typing it correctly difficult. 😞 I think we may be able to make this better using method overloads, but I don't want to block these changes on that. I'll add a TODO here to tackle this later. The return type though at least gives us some amount of type safety.

handeyeco

Another batch of comments. Will continue looking at this after my meetings.

[ note to self, I left off at

1503                // expr.terms should always have at least two terms
1504                const last = _.last(expr.terms)!;

]

packages/kas/src/nodes.ts

handeyeco · 2024-10-16T14:06:17Z

packages/kas/src/nodes.ts

        var args = _.map(this.args(), function (arg) {
-            return _.isString(arg) ? arg : arg[method].apply(arg, passed);
+            return _.isString(arg) || _.isNumber(arg)


Could you talk a little about the switch from isString to isString || isNumber? I see that in a couple of places. For the most part we're just changing types, but this is changing logic right?

Sometimes the args passed to node constructors can include numbers. This is the case for Num and its subclasses. I'm not sure why the existing code hasn't caused issues previously. 🤷‍♂️

handeyeco · 2024-10-16T14:07:14Z

packages/kas/src/nodes.ts

        return this.recurse("normalize");
    }

    // expands the expression
-    expand() {
+    expand(options?): Expr {


Since options isn't being used, can it be omitted?

Yes it can. I'll make this change.

packages/kas/src/nodes.ts

handeyeco · 2024-10-16T14:14:09Z

packages/kas/src/nodes.ts

@@ -293,7 +330,7 @@ class Expr {

    // raise this expression to a given exponent
    // most useful for eventually implementing i^3 = -i, etc.
-    raiseToThe(exp) {
+    raiseToThe(exp: Expr, options?: {preciseFloats?: boolean}): Expr {


Same here: do we need options if it's never used? Or do we need it in order to tell TS that this is implementing an abstract class or something?

We need it here because one of the subclasses uses it.

handeyeco · 2024-10-16T14:15:46Z

packages/kas/src/nodes.ts

-    needsExplicitMul() {
-        return this.args()[0].needsExplicitMul();
+    needsExplicitMul(): boolean {
+        return this.exprArgs()[0].needsExplicitMul();


Is this essentially the same, just more explicit?

Without it TS thinks the return type is any. Sometimes TS type inference is just really bad. I tried turning on noImplicitAny: true in the tsconfig.json but then it was complaining about things that it was able to infer as a real type. 😞

handeyeco · 2024-10-16T14:25:01Z

packages/kas/src/nodes.ts

+                    term instanceof Rational &&
+                    !(term instanceof Int) &&


nit: I kind of like having hasDenom as I think it makes intent clearer. Wouldn't lose sleep either way though.

I hear you. TS unfortunately isn't able to follow the logic when it's expressed that way.

packages/kas/src/nodes.ts

handeyeco

Insomuch as I understand KAS, I think this PR makes sense. Thanks for doing all of this! I just ask that you don't land this right before going on sabbatical because I'm not sure any LEMS people understand this as much as you. 😂

handeyeco · 2024-10-16T18:48:39Z

packages/kas/src/nodes.ts

@@ -1582,7 +1620,7 @@ export class Pow extends Expr {
            // If Float, convert to a Rational to enable the logic below
            if (simplifiedExp instanceof Float) {
                var num = simplifiedExp.n;
-                var decimals = (num - num.toFixed()).toString().length - 2;
+                var decimals = (num - +num.toFixed()).toString().length - 2;


This is just explicit coercion rather than implicit?

That's correct.

handeyeco · 2024-10-16T18:52:34Z

packages/kas/src/nodes.ts

+                    // a Num here but tbh I'm not sure how this code isn't causing
+                    // an infinite loop.


tbh I'm not sure how this code isn't causing an infinite loop.

lol, comforting

handeyeco · 2024-10-16T18:59:04Z

packages/kas/src/nodes.ts

+        const [base, exp] =
+            factor instanceof Pow
+                ? [factor.base, factor.exp]
+                : [factor, NumOne];


nit so feel free to disregard, but IMO it's weird to create objects/arrays just to destructure from them. I like the original, but this is also an option (and shorter):

Suggested change

const [base, exp] =

factor instanceof Pow

? [factor.base, factor.exp]

: [factor, NumOne];

const factorIsPow = factor instanceof Pow;

const base = factorIsPow ? factor.base : factor;

const exp = factorIsPow ? factor.exp : NumOne;

This is just a personal preference thing. Not going to lose sleep on it.

I don't think TS will refine factor to be a Pow correctly if we do that. We'd have to repeat the factor instanceof Pow in each ternary.

handeyeco · 2024-10-16T19:00:22Z

packages/kas/src/nodes.ts

        super();
        this.base = base;
        this.power = power;
-        this.hints = _.extend(this.hints, {
+        this.hints = {
+            ...this.hints,


These are hints from the parent class?

That's correct. I believe that Expr sets parens: false or something like that.

packages/kas/src/nodes.ts

handeyeco · 2024-10-16T19:19:54Z

packages/kas/src/nodes.ts

@@ -3553,13 +3615,15 @@ export const parse = function (input, options) {
        var expr = parser.parse(input).completeParse();
        return {parsed: true, expr: expr};
    } catch (e) {
-        return {parsed: false, error: e.message};
+        return {parsed: false, error: (e as Error).message};


Is this possible? I feel like probably not or else I imagine you would have done this.

} catch (e: Error) { return {parsed: false, error: e.message; }

TS does not allow this unfortunately. Part of the reason for that is that you can throw things other than Errors (or Error subclasses).

packages/kas/src/nodes.ts

…ction with fat arrow in a bunch of places

@kevinb-khan

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/[email protected] ### Patch Changes - [#1747](#1747) [`81ee69b0a`](81ee69b) Thanks [@kevinb-khan](https://github.com/kevinb-khan)! - Update nodes.js to use ES6 classes - [#1748](#1748) [`93bd39b6b`](93bd39b) Thanks [@kevinb-khan](https://github.com/kevinb-khan)! - Convert nodes.js to use TypeScript ## @khanacademy/[email protected] ### Patch Changes - [#1751](#1751) [`c95d08056`](c95d080) Thanks [@Myranae](https://github.com/Myranae)! - Refine InputNumber's rubric type - [#1756](#1756) [`3a208ba12`](3a208ba) Thanks [@Myranae](https://github.com/Myranae)! - Refine LabelImage's Rubric type - [#1762](#1762) [`a0f438fd7`](a0f438f) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - BUGFIX - [Number Line] - Some exercises with fractions wouldn't render - Updated dependencies \[[`81ee69b0a`](81ee69b), [`93bd39b6b`](93bd39b)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`81ee69b0a`](81ee69b), [`c95d08056`](c95d080), [`93bd39b6b`](93bd39b), [`3a208ba12`](3a208ba), [`a0f438fd7`](a0f438f)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`81ee69b0a`](81ee69b), [`93bd39b6b`](93bd39b)]: - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: mark-fitzgerald Required Reviewers: Approved By: mark-fitzgerald Checks: ⏭️ Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1755

kevinb-khan self-assigned this Oct 13, 2024

khan-actions-bot requested a review from a team October 13, 2024 18:58

kevinb-khan commented Oct 13, 2024

View reviewed changes

kevinb-khan force-pushed the kas-ts-nodes branch from 8f51d21 to a1aa77e Compare October 13, 2024 19:20

kevinb-khan force-pushed the kas-ts-nodes branch from 0ae9cfb to f38b16a Compare October 13, 2024 22:28

handeyeco reviewed Oct 15, 2024

View reviewed changes

handeyeco requested a review from a team October 15, 2024 21:32

handeyeco reviewed Oct 16, 2024

View reviewed changes

kevinb-khan requested a review from a team October 16, 2024 17:19

handeyeco approved these changes Oct 16, 2024

View reviewed changes

kevinb-khan requested a review from jeremywiebe October 16, 2024 21:06

kevinb-khan changed the base branch from kas-class-nodes to main October 17, 2024 15:02

An error occurred while trying to automatically change base from kas-class-nodes to main October 17, 2024 15:02

kevinb-khan added 14 commits October 17, 2024 11:05

rename nodes.js to nodes.ts

b296c02

convert nodes.js to TypeScript

cddb80b

add changeset file, format nodes.ts

bc2d684

move @ts-expect-error comment after formatting

a0a7093

annotate the functions dictionary in the Trig class

b344a9a

add types for vars and options params in eval() method sigs

1b8f80f

make some classes and methods 'abstract', add moar types, replace fun…

8c75089

…ction with fat arrow in a bunch of places

improve the types for 'func' and simplify name()

841f1d2

format

2faf4a2

replace Readonly<Expr> with Expr

77bfaf5

address feedback

4b9b769

remove reference to IE8 in one of the comments

3c0cabe

address more feedback

8170b68

hoist isAdd helper

d9d350e

kevinb-khan force-pushed the kas-ts-nodes branch from 77b359f to d9d350e Compare October 17, 2024 15:05

Add a comment about 'construct' signatures

00596dd

kevinb-khan merged commit 93bd39b into main Oct 17, 2024
9 checks passed

kevinb-khan deleted the kas-ts-nodes branch October 17, 2024 15:20

khan-actions-bot mentioned this pull request Oct 17, 2024

Version Packages #1755

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert nodes.js to TypeScript #1748

Convert nodes.js to TypeScript #1748

kevinb-khan commented Oct 13, 2024 •

edited

Loading

khan-actions-bot commented Oct 13, 2024 •

edited

Loading

github-actions bot commented Oct 13, 2024 •

edited

Loading

kevinb-khan Oct 13, 2024

kevinb-khan Oct 13, 2024

kevinb-khan Oct 13, 2024

github-actions bot commented Oct 13, 2024 •

edited

Loading

handeyeco left a comment

handeyeco Oct 15, 2024

kevinb-khan Oct 15, 2024

handeyeco Oct 15, 2024

kevinb-khan Oct 15, 2024

handeyeco left a comment

handeyeco Oct 16, 2024

kevinb-khan Oct 16, 2024

handeyeco Oct 16, 2024

kevinb-khan Oct 16, 2024

handeyeco Oct 16, 2024

kevinb-khan Oct 16, 2024

handeyeco Oct 16, 2024

kevinb-khan Oct 16, 2024

handeyeco Oct 16, 2024

kevinb-khan Oct 16, 2024

handeyeco left a comment

handeyeco Oct 16, 2024

kevinb-khan Oct 16, 2024

handeyeco Oct 16, 2024

handeyeco Oct 16, 2024

kevinb-khan Oct 16, 2024

handeyeco Oct 16, 2024

kevinb-khan Oct 16, 2024

handeyeco Oct 16, 2024

kevinb-khan Oct 16, 2024

		// a Num here but tbh I'm not sure how this code isn't causing
		// an infinite loop.

Convert nodes.js to TypeScript #1748

Convert nodes.js to TypeScript #1748

Conversation

kevinb-khan commented Oct 13, 2024 • edited Loading

Summary:

Test plan:

khan-actions-bot commented Oct 13, 2024 • edited Loading

Gerald

github-actions bot commented Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2024 • edited Loading

npm Snapshot: Published

handeyeco 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

handeyeco 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

Choose a reason for hiding this comment

handeyeco 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

kevinb-khan commented Oct 13, 2024 •

edited

Loading

khan-actions-bot commented Oct 13, 2024 •

edited

Loading

github-actions bot commented Oct 13, 2024 •

edited

Loading

github-actions bot commented Oct 13, 2024 •

edited

Loading