Skip to content

Stop bundling _any_ mathquill styles/fonts - let host app do that! #2385

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.

Sign up for GitHub

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

Merged
jeremywiebe merged 17 commits into from
Apr 21, 2025
Merged

Stop bundling _any_ mathquill styles/fonts - let host app do that! #2385

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
dd46e1e
Stop bundling _any_ mathquill styles/fonts - let host app do that!
jeremywiebe Apr 11, 2025
041cf94
docs(changeset): Stop bundling mathquill CSS - enables consuming app …
jeremywiebe Apr 11, 2025
244028f
WIP
jeremywiebe Apr 14, 2025
58b47fc
Use new build of mathquill (v1.0.2)
jeremywiebe Apr 15, 2025
acf44e1
Fix build so mathquill fonts are bundled!
jeremywiebe Apr 15, 2025
65203fc
Address stopship
jeremywiebe Apr 15, 2025
357c027
Move mathquill dep to be a devDependency (because we bundle it)
jeremywiebe Apr 15, 2025
0c069ea
Fix build so mathquill is not marked as external and we retain the #S…
jeremywiebe Apr 15, 2025
74b367e
Whew, hope this works now
jeremywiebe Apr 15, 2025
6db55ec
Fix rollup-plugin-auto-external to be exactly what it was before
jeremywiebe Apr 15, 2025
9facb1a
Revert change to `pnpm clean`
jeremywiebe Apr 16, 2025
9300471
Remove special case for devDependencies from math-input's .eslintrc.js
jeremywiebe Apr 16, 2025
3888003
I think we can just use the git tag for installs instead of a vendore…
jeremywiebe Apr 16, 2025
3d17c26
Update mathquill to v1.0.3
jeremywiebe Apr 16, 2025
e3d7f06
Simplify rollup-plugin-styles config for 'url()'s
jeremywiebe Apr 17, 2025
0ed73a4
Remove unused import
jeremywiebe Apr 17, 2025
60e9460
Final bits of readiness for mathquill fonts/css
jeremywiebe Apr 17, 2025
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/tame-grapes-ring.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/math-input": minor
---

Stop bundling mathquill CSS - enables consuming app to control bundling and font resolution
Copy link
Contributor

@handeyeco handeyeco Apr 17, 2025

Choose a reason for hiding this comment

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

I'm guessing we need to let Learning Equality know?

Copy link
Contributor

@handeyeco handeyeco Apr 17, 2025

Choose a reason for hiding this comment

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

Actually I'm a little confused by this changeset. Here you're saying "enables consuming app to control bundling and font resolution" but in the comment below you say "This resolve is for the fonts in Mathquill. We want to bundle them into the math-input package so that consumers of @khanacademy/math-input don't need to also depend on the mathquill package." The two comments seem at odds, right?

Copy link
Collaborator Author

@jeremywiebe jeremywiebe Apr 17, 2025

Choose a reason for hiding this comment

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

It gives consumers control by bundling the Mathquill fonts into the @khanacademy/math-input package complete with correct CSS files point to the bundled font files using relative paths.

The custom resolve was for Mathquill fonts, but after you asked, I found I was able to simplify things and didn't need a custom resolver (the built-in one handles things fine). And that was exactly the motivation for updating Mathquill! Thanks.

27 changes: 21 additions & 6 deletions config/build/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint-disable import/no-commonjs */
import fs from "fs";
import {URL} from "node:url";
import path from "path";

import alias from "@rollup/plugin-alias";
Expand Down Expand Up @@ -149,12 +150,26 @@ const createConfig = (
}),
styles({
mode: "extract",
// We don't want to try to resolve the url() occurrences in our
// stylesheets. We'll leave that for consumers of the library
// to deal with. Otherwise we end up packaging upstream assets
// into our libraries when our consumers should be the ones
// handling asset bundling.
url: false,
minimize: true,
url: {
publicPath: "./assets",
// This resolve is for the fonts in Mathquill. We want to
// bundle them into the math-input package so that
// consumers of @khanacademy/math-input don't need to
// _also_ depend on the mathquill package.
resolve(inputUrl, baseDir) {
// Some inputUrl's have a hash (eg.
// font/Symbola.svg#Symbola). We use the URL object to
// ensure we only have the file path and nothing else.
const url = new URL(inputUrl, "http://example.com");
Copy link
Collaborator Author

@jeremywiebe jeremywiebe Apr 16, 2025

Choose a reason for hiding this comment

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

I used the URL object here instead of string searching just to ensure we handle things correctly.

const targetFile = path.join(baseDir, url.pathname);
return {
from: targetFile,
source: fs.readFileSync(targetFile),
Copy link

Copilot AI Apr 16, 2025

Choose a reason for hiding this comment

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

Consider adding error handling for fs.readFileSync to handle cases where a font file may be missing, which would otherwise cause a runtime error during the build.

Suggested change
return {
from: targetFile,
source: fs.readFileSync(targetFile),
let fileSource;
try {
fileSource = fs.readFileSync(targetFile);
} catch (error) {
console.error(`Error reading file: ${targetFile}`);
console.error(error.message);
throw error;
}
return {
from: targetFile,
source: fileSource,

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@jeremywiebe jeremywiebe Apr 16, 2025

Choose a reason for hiding this comment

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

It's safe to throw here as Rollup will capture the Error and fail the build with a sensible error.

urlQuery: url.hash,
};
},
},
less: {
math: "always",
},
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"@storybook/react-vite": "^8.6.11",
"@swc-node/register": "^1.10.10",
"@swc/core": "1.11.13",
"@swc/helpers": "^0.5.17",
Copy link
Collaborator Author

@jeremywiebe jeremywiebe Apr 16, 2025

Choose a reason for hiding this comment

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

I saw warnings that a few files couldn't resolve this module. I'm not sure why it didn't happen earlier, but adding this to ensure our builds are correct.

"@swc/jest": "^0.2.37",
"@testing-library/dom": "^10.3.1",
"@testing-library/jest-dom": "^6.4.6",
Expand Down
2 changes: 1 addition & 1 deletion packages/math-input/less/main.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
@import "./overrides.less";
@import (inline) "../node_modules/mathquill/build/mathquill.css";
@import "mathquill/build/mathquill.css";
Copy link
Collaborator Author

@jeremywiebe jeremywiebe Apr 16, 2025

Choose a reason for hiding this comment

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

This is the correct import syntax to resolve a CSS/Less file out of an dependant npm package.

4 changes: 2 additions & 2 deletions packages/math-input/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@
"dependencies": {
"@khanacademy/keypad-context": "workspace:*",
"@khanacademy/perseus-core": "workspace:*",
"@khanacademy/perseus-utils": "workspace:*",
"mathquill": "file:../../vendor/mathquill-v1.0.0.tgz"
Copy link
Collaborator Author

@jeremywiebe jeremywiebe Apr 16, 2025

Choose a reason for hiding this comment

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

We move this to be in devDependencies because we now bundle everything from mathquill into @khanacademy/math-input. This relieves client apps from needing to depend on mathquill directly (most client apps don't use mathquill directly if they're using @khanacademy/math-input)

"@khanacademy/perseus-utils": "workspace:*"
},
"devDependencies": {
"@khanacademy/mathjax-renderer": "catalog:",
Expand All @@ -57,6 +56,7 @@
"aphrodite": "catalog:",
"jquery": "catalog:",
"katex": "0.11.1",
"mathquill": "file:../../vendor/mathquill-v1.0.2.tgz",
"perseus-build-settings": "workspace:*",
"prop-types": "catalog:",
"react": "catalog:",
Expand Down
2 changes: 1 addition & 1 deletion packages/perseus/src/styles/perseus-renderer.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,4 +714,4 @@ div.graphie {
padding-top: 0;
}

@import "../../../math-input/less/main.less";
@import (reference) "@khanacademy/math-input/styles.css";
Copy link
Collaborator Author

@jeremywiebe jeremywiebe Apr 16, 2025

Choose a reason for hiding this comment

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

This becomes a (reference) now to avoid the situation where an app using math-input directly would "double" the math-input CSS because it's integrated into the Perseus CSS also.

This means that consuming apps will need to ensure that math-input CSS is properly imported, but that's a one-line change and allows for deduplication of math-input CSS.

Loading
Loading