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 all 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
32 changes: 28 additions & 4 deletions packages/perseus/src/__tests__/renderer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,6 @@ describe("renderer", () => {
expect(screen.queryAllByRole("img")).toHaveLength(1);

const wrapperStyle = getComputedStyle(
// $FlowIgnore[incompatible-call]
// @ts-expect-error - TS2345 - Argument of type 'HTMLElement | null' is not assignable to parameter of type 'Element'.
screen.getByAltText("This image has dimensions").parentElement, // eslint-disable-line testing-library/no-node-access
);
Expand Down Expand Up @@ -714,6 +713,29 @@ describe("renderer", () => {
it("should replace deprecated alignment tags in inline math", async () => {
// Arrange
const question = {
content:
"Hello $\\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
await waitFor(() => {
expect(
screen.getByText(/\\begin\{aligned\}.*\\end\{aligned\}/),
).toBeInTheDocument();
});
});

it("should replace deprecated alignment tags in block math", async () => {
// Arrange
const question = {
// Math that exists by itself in a paragraph is considered
// block math, even if it isn't surrounded by the block math
// delimeters (`$$$...$$$`).
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: {},
Expand All @@ -724,9 +746,11 @@ describe("renderer", () => {
renderQuestion(question);

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

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
5 changes: 4 additions & 1 deletion 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 @@ -1293,6 +1293,9 @@ class Renderer extends React.Component<Props, State> {
// 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.
// TODO(LEMS-1608) Remove this replacement as MathJax supports the
// "align" macro correctly (and, in fact, it is not synonymous with
// "aligned").
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
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
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
/**
* Preprocess TeX code to convert things that KaTeX doesn't know how to handle
* 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 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.
// TODO(LEMS-1608) Remove this replacement as MathJax supports the
// "align" macro correctly (and, in fact, it is not synonymous with
// "aligned").
.replace(/\{align[*]?\}/g, "{aligned}")
// 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 to
// 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