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

Remove 'katex' from @khanacademy/perseus #1428

Merged
merged 8 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/mighty-news-repair.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Drop katex dependency - no longer used
1 change: 0 additions & 1 deletion packages/perseus/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@
"create-react-class": "15.6.3",
"intersection-observer": "^0.12.0",
"jquery": "^2.1.1",
"katex": "0.11.1",
"lodash.debounce": "^4.0.8",
"perseus-build-settings": "^0.4.1",
"prop-types": "15.6.1",
Expand Down
18 changes: 0 additions & 18 deletions packages/perseus/src/__tests__/renderer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -710,24 +710,6 @@ describe("renderer", () => {
});
expect(container).toMatchSnapshot();
});

it("should replace deprecated alignment tags in inline math", async () => {
// Arrange
const question = {
content:
"$\\begin{align}\n2\\text{HCl}(\\text{aq})+\\text{Ca}(\\text{OH})_2(\\text{aq})\\rightarrow\\text{Ca}(\\text{s})+2\\text H_2\\text O(\\text l)+\\text{Cl}_2(\\text g)\n\\end{align}$",
images: {},
widgets: {},
} as const;

// Act
renderQuestion(question);

// Assert
expect(
screen.getByText(/\\begin\{aligned\}.*\\end\{aligned\}/),
).toBeInTheDocument();
});
});

// Note that we can't use `.each` here because the three props require
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import * as React from "react";

import Graph from "../graph";
// TODO(scottgrant): Katex is unavailable here. Fix!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't even make sense of what was to be done here so I'm deleting this comment.


type StoryArgs = Record<any, any>;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ const ForceZoomWrapper = ({children}: Props): React.ReactElement => (
</>
);

export const KaTeX = (args: StoryArgs): React.ReactElement => {
export const Tex = (args: StoryArgs): React.ReactElement => {
return (
<ForceZoomWrapper>
<ZoomableTex children="\sum_{i=1}^\infty\frac{1}{n^2} =\frac{\pi^2}{6}" />
</ForceZoomWrapper>
);
};

export const ComplexKaTeX = (args: StoryArgs): React.ReactElement => {
export const ComplexTex = (args: StoryArgs): React.ReactElement => {
return (
<ForceZoomWrapper>
{" "}
Expand Down
2 changes: 1 addition & 1 deletion packages/perseus/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export {
*/
export {default as Util} from "./util";
export {default as KhanColors} from "./util/colors";
export {default as preprocessTex} from "./util/katex-preprocess";
export {default as preprocessTex} from "./util/tex-preprocess";
export {registerAllWidgetsForTesting} from "./util/register-all-widgets-for-testing";
export * as SizingUtils from "./util/sizing-utils";
export {
Expand Down
6 changes: 3 additions & 3 deletions packages/perseus/src/interactive2/movable-point.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -519,9 +519,9 @@
* Displays a tooltip above the point, replacing any previous contents. If
* there is no tooltip initialized, adds the tooltip.
*
* If the type of contents is string, the contents will be rendered with
* KaTeX. Otherwise, the content will be assumed to be a DOM node and will
* be appended inside the tooltip.
* If the type of contents is string, the contents will be rendered as TeX
Copy link
Member

Choose a reason for hiding this comment

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

Nit: TeX is the input language, so I think it would be more accurate to say that "the contents will be interpreted as TeX and rendered as DOM elements"

* Otherwise, the content will be assumed to be a DOM node and will be

Check warning on line 523 in packages/perseus/src/interactive2/movable-point.tsx

View check run for this annotation

Codecov / codecov/patch

packages/perseus/src/interactive2/movable-point.tsx#L522-L523

Added lines #L522 - L523 were not covered by tests
* appended inside the tooltip.
*/
_showTooltip(contents) {
if (!this._tooltip) {
Expand Down
4 changes: 2 additions & 2 deletions packages/perseus/src/perseus-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ export const PerseusExpressionAnswerFormConsidered = [
] as const;

export type PerseusExpressionAnswerForm = {
// The Katex form of the expression. e.g. "x\\cdot3=y"
// The TeX form of the expression. e.g. "x\\cdot3=y"
value: string;
// The Answer expression must have the same form
form: boolean;
Expand Down Expand Up @@ -874,7 +874,7 @@ export type PerseusGraphTypeRay = {
} & PerseusGraphTypeCommon;

export type PerseusLabelImageWidgetOptions = {
// Translatable Text; Katex representation of choices
// Translatable Text; Tex representation of choices
choices: ReadonlyArray<string>;
// The URL of the image
imageUrl: string;
Expand Down
10 changes: 2 additions & 8 deletions packages/perseus/src/renderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import PerseusMarkdown from "./perseus-markdown";
import QuestionParagraph from "./question-paragraph";
import TranslationLinter from "./translation-linter";
import Util from "./util";
import preprocessTex from "./util/katex-preprocess";
import preprocessTex from "./util/tex-preprocess";
import WidgetContainer from "./widget-container";
import * as Widgets from "./widgets";

Expand Down Expand Up @@ -1289,12 +1289,6 @@ class Renderer extends React.Component<Props, State> {
);
}
if (node.type === "math") {
// Replace uses of \begin{align}...\end{align} which KaTeX doesn't
// support (yet) with \begin{aligned}...\end{aligned} which renders
// the same is supported by KaTeX. It does the same for align*.
// TODO(kevinb) update content to use aligned instead of align.
const tex = node.content.replace(/\{align[*]?\}/g, "{aligned}");
Copy link
Collaborator Author

@jeremywiebe jeremywiebe Jul 18, 2024

Choose a reason for hiding this comment

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

MathJax supports the \begin{align} macro just fine (tested in Storybook).

Copy link
Member

@benchristel benchristel Jul 19, 2024

Choose a reason for hiding this comment

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

Removing this code might break the exercise editor, which still uses KaTeX behind the scenes to validate that the input TeX can render with KaTeX. This gets complicated to reason about, because the reason the editor does that is to ensure that TeX written by content creators is compatible with old versions of the mobile app, which use KaTeX (and old versions of Perseus which have this replacement logic, so \begin{align} will work fine there).

I think we could move this logic to the exercise editor and it would be fine. But I'd lean toward keeping this code the same until we can remove KaTeX everywhere. If you agree, can you restore this code and add a TODO comment here referencing LEMS-1608?

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I looked up our new mobile deprecation policy and I believe we will be able to completely remove KaTeX starting September 18th!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to move forward and land this, so I'll do as Ben suggests and restore this code with a new TODO. Thanks for thinking of that Ben!


// We render math here instead of in perseus-markdown.jsx
// because we need to pass it our onRender callback.
return (
Expand All @@ -1316,7 +1310,7 @@ class Renderer extends React.Component<Props, State> {
onRender={this.props.onRender}
setAssetStatus={setAssetStatus}
>
{tex}
{node.content}
</TeX>
)}
</AssetContext.Consumer>
Expand Down
1 change: 0 additions & 1 deletion packages/perseus/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,6 @@ export type APIOptions = Readonly<{

type TeXProps = {
children: string;
katexOptions?: any;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked and this prop/string is not used in webapp anywhere!

onClick?: () => unknown;
onRender?: (root?: any) => unknown;
style?: any;
Expand Down
14 changes: 0 additions & 14 deletions packages/perseus/src/util/katex-preprocess.ts

This file was deleted.

8 changes: 8 additions & 0 deletions packages/perseus/src/util/tex-preprocess.ts
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Git(hub) doesn't seem to recognize that this is a rename.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* Preprocess TeX code to convert things that MathJax doesn't know how to handle
* to things is does.
*/
export default (texCode: string): string =>
texCode
// Replace non-breaking spaces with regular spaces.
.replace(/[\u00a0]/g, " ");
4 changes: 2 additions & 2 deletions packages/perseus/src/util/tex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ function findChildOrAdd(elem: any, className: string) {
}

export default {
// Process a node and add math inside of it. This attempts to use KaTeX to
// format the math, and if that fails it falls back to MathJax.
// Process a node and add math inside of it. This uses MathJax
jeremywiebe marked this conversation as resolved.
Show resolved Hide resolved
// format the math.
//
// elem: The element which the math should be added to.
//
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {usePerseusI18n} from "../../components/i18n-context";
import Renderer from "../../renderer";

export type AnswerType = {
// The answer string, can be plain text or a KaTeX expression.
// The answer string, can be plain text or a TeX expression.
content: string;
// Whether the answer is selected.
checked: boolean;
Expand Down