Update KAS nodes.js to use ES6 classes #1747

kevinb-khan · 2024-10-13T14:37:09Z

Summary:

I had tried this previously but failed because I tried to convert all of the classes to ES6 classes at once. This time around I converted them one at a time and made sure that all of the tests were passing after each class was converted. In the next PR I will convert this file to TypeScript.

Issue: None

Test plan:

yarn test packages/kas/src

khan-actions-bot · 2024-10-13T14:37:23Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/fifty-queens-teach.md, packages/kas/src/nodes.js

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-13T14:41:10Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1747

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

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

github-actions · 2024-10-13T14:41:14Z

Size Change: -262 B (-0.03%)

Total Size: 865 kB

Filename	Size	Change
`packages/kas/dist/es/index.js`	38 kB	-262 B (-0.68%)

ℹ️ 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`	419 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-15T01:08:40Z

packages/kas/src/nodes.js

-// static methods for the sequence types
-_.each([Add, Mul], function (type) {
-    _.extend(type, {
-        // create a new sequence unless left is already one (returns a copy)
-        createOrAppend: function (left, right) {
-            if (left instanceof type) {
-                return new type(left.terms.concat(right));
-            } else {
-                return new type(left, right);
-            }
-        },
-    });
-});


These were added directly to Add and Mul as static methods.

kevinb-khan · 2024-10-15T01:09:25Z

packages/kas/src/nodes.js

-    // hints for interpreting and rendering user input
-    hints: _.extend(Num.prototype.hints, {
-        negate: false,
-        subtract: false,
-        divide: false,
-        root: false,
-        fraction: false,
-        entered: false,
-    }),


This was moved into the constructor.

kevinb-khan · 2024-10-15T01:09:52Z

packages/kas/src/nodes.js

-// static methods and fields that are best defined on Num
-_.extend(Num, {
-    negativeOne: function (hint) {


These were made static methods on Num.

kevinb-khan · 2024-10-15T01:10:25Z

packages/kas/src/nodes.js

-    hints: _.extend(Log.prototype.hints, {
-        open: false,
-    }),


This was moved into the constructor.

kevinb-khan · 2024-10-15T01:10:41Z

packages/kas/src/nodes.js

-    hints: _.extend(Trig.prototype.hints, {
-        open: false,
-    }),


This was moved into the constructor.

kevinb-khan · 2024-10-15T01:10:52Z

packages/kas/src/nodes.js

-    hints: {
-        parens: false,
-    },


This was moved into the constructor.

kevinb-khan · 2024-10-15T01:10:58Z

packages/kas/src/nodes.js

+    // hints = {
+    //     parens: false,
+    // };


This can be deleted.

Makes sense, do you want to remove it?

Yes, I'll delete it's before landing.

I checked and it's removed in the TS PR. Once I land both of them, this'll go away.

handeyeco

Tremendous work; I can't express how grateful I am that you took on this effort.

handeyeco · 2024-10-15T20:51:23Z

packages/kas/src/nodes.js

+    // hints = {
+    //     parens: false,
+    // };


Makes sense, do you want to remove it?

handeyeco · 2024-10-15T20:55:31Z

packages/kas/src/nodes.js

-
-/* sequence of additive terms */
-export function Add() {
-    if (arguments.length === 1) {


My understanding is it's fine to remove this since it's a par of Seq?

That's right. I moved this to the Seq constructor because we do the same thing for both Add and Mul.

handeyeco · 2024-10-15T20:59:52Z

packages/kas/src/nodes.js

+}
+
+// static methods for the sequence types
+_.each([Add, Mul], function (cls) {


I'd almost rather have duplicate code in the classes than adding the static methods in this kind of non-standard way. I guess the worry is they'd diverge maybe? I know this isn't a result of your PR since we were doing this before, but I wonder if this is the right time to move this?

I do exactly this in the TypeScript PR since TypeScript doesn't like you adding (static) methods to classes after the fact.

jeremywiebe

I've also taken runs at this file in attempts to convert it to classes and failed! Nice!!! :)

## 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 `var`s 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 Author: kevinb-khan Reviewers: kevinb-khan, handeyeco, jeremywiebe Required Reviewers: Approved By: handeyeco Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1748

@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

Update KAS nodes.js to use ES6 classes

cb76a65

kevinb-khan self-assigned this Oct 13, 2024

khan-actions-bot requested a review from a team October 13, 2024 14:37

add changeset file

c230e64

kevinb-khan mentioned this pull request Oct 13, 2024

Convert nodes.js to TypeScript #1748

Merged

format nodes.js

fab2a7c

kevinb-khan commented Oct 15, 2024

View reviewed changes

handeyeco approved these changes Oct 15, 2024

View reviewed changes

jeremywiebe approved these changes Oct 15, 2024

View reviewed changes

kevinb-khan merged commit 81ee69b into main Oct 17, 2024
9 checks passed

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

Update KAS nodes.js to use ES6 classes #1747

Update KAS nodes.js to use ES6 classes #1747

kevinb-khan commented Oct 13, 2024

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

kevinb-khan Oct 15, 2024

kevinb-khan Oct 15, 2024

kevinb-khan Oct 15, 2024

kevinb-khan Oct 15, 2024

kevinb-khan Oct 15, 2024

kevinb-khan Oct 15, 2024

kevinb-khan Oct 15, 2024

handeyeco Oct 15, 2024

kevinb-khan Oct 15, 2024

kevinb-khan Oct 17, 2024

handeyeco left a comment

handeyeco Oct 15, 2024

handeyeco Oct 15, 2024

kevinb-khan Oct 15, 2024

handeyeco Oct 15, 2024

kevinb-khan Oct 15, 2024

jeremywiebe left a comment

Update KAS nodes.js to use ES6 classes #1747

Update KAS nodes.js to use ES6 classes #1747

Conversation

kevinb-khan commented Oct 13, 2024

Summary:

Test plan:

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

Gerald

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

npm Snapshot: Published

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

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

jeremywiebe left a comment

Choose a reason for hiding this comment

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