Init `perseus-score` and move `answer-types` and `perseus-types` #2086

handeyeco · 2025-01-08T20:40:23Z

Summary:

This PR is to initialize a new subpackage: perseus-score

To prove that the package was working, I decided to move answer-types.ts from perseus to perseus-score. This resulted in a couple of side-effects (as a result of the fact that perseus-score cannot import from perseus):

I needed to move util/math.ts from perseus to kmath
math.ts needed something from perseus-types.ts which means I needed to go ahead and move that from perseus to perseus-core (and per Jeremy's request I renamed perseus-types.ts to data-schema.ts)
Probably 90%+ of the files changed are because of the perseus-types.ts move
answer-types.ts needed access to strings.ts which is a special export from perseus, so I had to refactor answer-types.ts to use error placeholders that could get mapped to strings when being displayed to learners.

Issue: LEMS-2737

Test plan:

Make sure widgets that use answer-types.ts still work
- Expression
- InputNumber
- Matrix
- NumericInput
- Table
Make sure user-facing errors in those widgets still work
I dunno, it's a big change; everything should still work

github-actions · 2025-01-08T21:04:02Z

Size Change: +172 kB (+13.5%) ⚠️

Total Size: 1.45 MB

Filename	Size	Change
`packages/kmath/dist/es/index.js`	83.1 kB	+78.8 kB (+1847.35%)	🆘
`packages/perseus-core/dist/es/index.js`	4.01 kB	+2.53 kB (+171.06%)	🆘
`packages/perseus-editor/dist/es/index.js`	688 kB	-1 B (0%)
`packages/perseus/dist/es/index.js`	407 kB	-12 kB (-2.87%)
`packages/perseus/dist/es/strings.js`	4.84 kB	+201 B (+4.33%)
`packages/perseus-score/dist/es/index.js`	103 kB	+103 kB (new file)	🆕

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/math-input/dist/es/index.js`	78 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

… perseus-score, move perseus-types in perseus to data-schema in perseus-core

github-actions · 2025-01-09T15:56:01Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR2086

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

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

handeyeco · 2025-01-09T16:05:46Z

packages/perseus-core/src/index.ts

@@ -13,3 +13,5 @@ export {libVersion} from "./version";

 export {Errors} from "./error/errors";
 export {PerseusError} from "./error/perseus-error";
+
+export * from "./data-schema";


Maybe this should be export * as DataSchema? It would just be very painful to do without figuring out a way of automating the change because it wouldn't be a simple find/replace.

I like the idea. I think it's something we could tackle as a follow-up.

handeyeco · 2025-01-09T16:07:25Z

packages/perseus-score/src/error-identifiers.ts

@@ -0,0 +1,13 @@
+function mark(error: string) {
+    return "\u2603 " + error + " \u2603";


Maybe this is dumb, but I wanted to make it clear that these were not meant to be learner-facing. They're essentially enums. I thought about using numeric error codes, but I thought it would be clearer to use an enum-like system.

That makes sense. One suggestion is to avoid using the snowman sigil. That has extremely special meaning in Perseus and I'd like to avoid folks seeing these values and getting confused, thinking they're widget references.

handeyeco · 2025-01-09T16:08:33Z

packages/perseus-score/src/util/answer-types.ts

@@ -571,13 +577,12 @@ const KhanAnswerTypes = {
                            } else if (form === "percent") {
                                // Otherwise, an error was returned
                                score.empty = true;
-                                score.message = strings.MISSING_PERCENT_ERROR;
+                                score.message = MISSING_PERCENT_ERROR;


So these are now the "error placeholder" vs the error message itself. There's a helper to map placeholders to translated strings.

handeyeco · 2025-01-09T16:08:59Z

packages/perseus-score/tsconfig-build.json

+        "rootDir": "src",
+        "paths": {
+            // NOTE(kevinb): We have to repeat this here because TS doesn't do
+            // intelligent merge of tsconfig.json files when using `extends`.


This is just copy-pasta, I don't know what I'm doing.

handeyeco · 2025-01-09T16:11:56Z

packages/perseus/src/strings.ts

+    if (!err) {
+        return err;
+    }
+    return errorToString[err] || err;


This allows us to take any error message and if it's an error placeholder, swap it out for a translated message.

So in that case we might show the learner an english string?

handeyeco · 2025-01-09T16:15:25Z

packages/perseus/src/widgets/input-number/input-number.test.ts

-            }
-        `);
+        expect(err).toEqual({
+            message: "☃ EXTRA_SYMBOLS_ERROR ☃",


This is what an error placeholder looks like in my head.

That seems reasonable. I got to thinking that maybe part of the reason we are thinking there's need for markers is that the field is still called message. If it were messageCode would that make things clearer and remove the need for the markers?

No change required here as this PR is big enough, but a potential for a future PR if you agree.

handeyeco · 2025-01-09T16:17:32Z

packages/perseus-score/src/util/answer-types.ts

@@ -100,7 +107,6 @@ const KhanAnswerTypes = {
        createValidatorFunctional: function (
            predicate: Predicate,
            options: any,
-            strings: PerseusStrings,


Since it's now using error placeholders instead of translated strings, we don't need to weave through the strings object.

Nice! I was thinking about how this will work out on the frontend and I think your choice of string codes over numeric will be useful. If we ever have a code that's not represented on the frontend, we'll at least have a code that reveals something about what happened on the server.

jeremywiebe

I'm pretty happy with how this is going! Thanks for all the yak-shaving to update imports!

One thing to do is run ./utils/pre-publish-check-ci.ts and fix any issues it raises. We don't run that per-PR but do run it when we publish. I think there's one issue that needs fixing with the new package.

It would probably be good to run this check as part of a Lint/Test check per PR just to catch these issues before we get to publish and blow it up.

jeremywiebe · 2025-01-09T23:15:55Z

packages/kmath/src/index.ts

@@ -5,3 +5,5 @@ export * as vector from "./vector";
 export * as point from "./point";
 export * as line from "./line";
 export * as ray from "./ray";
+
+export {default as KhanMath, sum} from "./math";


Exporting sum here introduces a duplicate function now in that vector.ts has an arraySum() that is exactly the same thing.

Maybe we can de-duplicate in a follow-up PR?

jeremywiebe · 2025-01-09T23:18:45Z

packages/perseus-core/src/data-schema.ts

Thanks for renaming this on the move.

jeremywiebe · 2025-01-09T23:24:05Z

packages/perseus-core/src/index.ts

@@ -13,3 +13,5 @@ export {libVersion} from "./version";

 export {Errors} from "./error/errors";
 export {PerseusError} from "./error/perseus-error";
+
+export * from "./data-schema";


I like the idea. I think it's something we could tackle as a follow-up.

jeremywiebe · 2025-01-09T23:28:02Z

packages/perseus-score/src/error-identifiers.ts

@@ -0,0 +1,13 @@
+function mark(error: string) {
+    return "\u2603 " + error + " \u2603";


That makes sense. One suggestion is to avoid using the snowman sigil. That has extremely special meaning in Perseus and I'd like to avoid folks seeing these values and getting confused, thinking they're widget references.

jeremywiebe · 2025-01-10T01:29:52Z

packages/kmath/src/math.ts

-import type {MathFormat} from "../perseus-types";
+import {number as knumber} from "@khanacademy/kmath";
+
+import type {MathFormat} from "@khanacademy/perseus-core";

 const KhanMath = {


For a future PR, I think a small cleanup that'd fit these Khanmath functions into this library would be to move the functions to number.ts or vector.ts (as appropriate). It's a nitpicky thing, but it would make this library more consumable instead of having a grab-bag of functions left in KhanMath.

Probably low priority, but if you feel like it... :)

jeremywiebe · 2025-01-10T01:38:28Z

packages/perseus-score/src/util/answer-types.ts

@@ -100,7 +107,6 @@ const KhanAnswerTypes = {
        createValidatorFunctional: function (
            predicate: Predicate,
            options: any,
-            strings: PerseusStrings,


Nice! I was thinking about how this will work out on the frontend and I think your choice of string codes over numeric will be useful. If we ever have a code that's not represented on the frontend, we'll at least have a code that reveals something about what happened on the server.

jeremywiebe · 2025-01-10T01:43:35Z

packages/perseus/src/strings.ts

+    APPROXIMATED_PI_ERROR,
+    EXTRA_SYMBOLS_ERROR,
+    MISSING_PERCENT_ERROR,
+    NEEDS_TO_BE_SIMPLIFIED_ERROR,
+    WRONG_CASE_ERROR,
+    WRONG_LETTER_ERROR,
+    MULTIPLICATION_SIGN_ERROR,


Seeing these sitting at the root of the perseus-core namespace concerns me as I read this file now.

What do you think of nesting them in a ScoringErrorCodes namespace? That will also allow us to put some type safety on the mapping code to help us make sure we've got a mapping for each error code (of course, that won't help us if we have mismatched library versions between the client and server.

jeremywiebe · 2025-01-10T01:45:23Z

packages/perseus/src/strings.ts

+    if (!err) {
+        return err;
+    }
+    return errorToString[err] || err;


So in that case we might show the learner an english string?

jeremywiebe · 2025-01-10T01:55:11Z

packages/perseus/src/widgets/input-number/input-number.test.ts

-            }
-        `);
+        expect(err).toEqual({
+            message: "☃ EXTRA_SYMBOLS_ERROR ☃",


That seems reasonable. I got to thinking that maybe part of the reason we are thinking there's need for markers is that the field is still called message. If it were messageCode would that make things clearer and remove the need for the markers?

No change required here as this PR is big enough, but a potential for a future PR if you agree.

handeyeco added 2 commits January 8, 2025 13:44

init perseus-score

01f3ad3

move answer-types to perseus-score

653463d

handeyeco self-assigned this Jan 8, 2025

remove yarn slip

23b84d8

handeyeco changed the title ~~move answer-types to perseus-score~~ Init perseus-score and move answer-types and perseus-types Jan 8, 2025

hail mary

cdccce1

handeyeco added 4 commits January 8, 2025 15:10

try something else

29514fc

keep trying

27d6ec5

switch from error strings to error identifiers

fb688d7

docs(changeset): Init perseus-score, move AnswerTypes from perseus to…

e7a5e47

… perseus-score, move perseus-types in perseus to data-schema in perseus-core

handeyeco marked this pull request as ready for review January 9, 2025 16:04

handeyeco commented Jan 9, 2025

View reviewed changes

handeyeco requested review from a team and jeremywiebe January 9, 2025 16:18

handeyeco mentioned this pull request Jan 9, 2025

Export Item and Article parsing functions from the 'perseus' package #2085

Open

jeremywiebe approved these changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init `perseus-score` and move `answer-types` and `perseus-types` #2086

Init `perseus-score` and move `answer-types` and `perseus-types` #2086

handeyeco commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025

handeyeco Jan 9, 2025

jeremywiebe Jan 9, 2025

handeyeco Jan 9, 2025 •

edited

Loading

jeremywiebe Jan 9, 2025

handeyeco Jan 9, 2025

handeyeco Jan 9, 2025

handeyeco Jan 9, 2025

jeremywiebe Jan 10, 2025

handeyeco Jan 9, 2025

jeremywiebe Jan 10, 2025

handeyeco Jan 9, 2025

jeremywiebe Jan 10, 2025

jeremywiebe left a comment

jeremywiebe Jan 9, 2025

jeremywiebe Jan 9, 2025

jeremywiebe Jan 9, 2025

jeremywiebe Jan 9, 2025

jeremywiebe Jan 10, 2025

jeremywiebe Jan 10, 2025

jeremywiebe Jan 10, 2025

jeremywiebe Jan 10, 2025

jeremywiebe Jan 10, 2025

		@@ -0,0 +1,13 @@
		function mark(error: string) {
		return "\u2603 " + error + " \u2603";

Init perseus-score and move answer-types and perseus-types #2086

Are you sure you want to change the base?

Init perseus-score and move answer-types and perseus-types #2086

Conversation

handeyeco commented Jan 8, 2025 • edited Loading

Summary:

Test plan:

github-actions bot commented Jan 8, 2025 • edited Loading

github-actions bot commented Jan 9, 2025

npm Snapshot: Published

Choose a reason for hiding this comment

Choose a reason for hiding this comment

handeyeco Jan 9, 2025 • 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

jeremywiebe 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

Init `perseus-score` and move `answer-types` and `perseus-types` #2086

Init `perseus-score` and move `answer-types` and `perseus-types` #2086

handeyeco commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

handeyeco Jan 9, 2025 •

edited

Loading