Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

benchristel
Copy link
Member

@benchristel benchristel commented Jan 8, 2025

This is the public API for Perseus JSON parsing that we'll call in Webapp.
See ADR 773 for context on why this is needed.

Issue: https://khanacademy.atlassian.net/browse/LEMS-2774

Test plan:

yarn test

Copy link
Contributor

github-actions bot commented Jan 8, 2025

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR2085

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

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

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Size Change: +916 B (+0.07%)

Total Size: 1.28 MB

Filename Size Change
packages/perseus/dist/es/index.js 420 kB +916 B (+0.22%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 39 kB
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 689 kB
packages/perseus-linter/dist/es/index.js 22.2 kB
packages/perseus/dist/es/strings.js 4.64 kB
packages/pure-markdown/dist/es/index.js 3.66 kB
packages/simple-markdown/dist/es/index.js 12.5 kB

compressed-size-action

@handeyeco
Copy link
Contributor

handeyeco commented Jan 9, 2025

I haven't looked at this yet, but my first reaction is that I think this will need to live outside of perseus if we ever want to use it on the server. My current work is moving logic we'll need on the server out of perseus and this seems like something we'll want in the medium-to-long term.

My guess is we'll want this in perseus-core. CC @jeremywiebe

(EDIT: I don't think we have to do it now, I'm just wondering if we should rip the band-aid while the work is fresh)

@benchristel
Copy link
Member Author

@handeyeco do you mean that all the parsing code will have to live in perseus-core? Let me double-check that it doesn't have any runtime dependencies on perseus...

@benchristel
Copy link
Member Author

There are a few trivial non-type imports from perseus-types that we could remove by duplicating the values in the parsing code.

import {lockedFigureColorNames} from "../../../perseus-types";
import {ItemExtras} from "../../../perseus-types";
import {plotterPlotTypes} from "../../../perseus-types";

More concerning: the parsing code imports getWidget, which it uses to find dynamically-registered widget types.

import {getWidget} from "../../../widgets";

Dynamically-defined widgets can't exist on the server (probably? I'm assuming that LEMS owns the server code and it's separate from services/static code), so I wonder if we should deprecate dynamic widgets entirely and say that only Perseus can define widgets.

@handeyeco
Copy link
Contributor

handeyeco commented Jan 9, 2025

perseus-types.ts in perseus is moving to data-schema.ts in perseus-core: #2086

We're going to have two registries, one on the BE (upgrading, scoring, validation) and one on the FE (upgrading, rendering).

Things might be too much in-flight to address right now.

import type {Parser} from "../parser-types";
import type {PerseusImageBackground} from "@khanacademy/perseus";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen "self" imports like this sneak into our codebase before. I think they work, but it'd be nice if we could lint for them and prevent them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants