Skip to content

Conversation

@zorkow
Copy link
Member

@zorkow zorkow commented Jun 5, 2025

PR primarily updates all packages and adds a github action to automatically run tests for PRs into develop branch or when pushing directly to develop. For all this it does quite a bit of housekeeping:

Changes in Main Code

  • Updates packages to the latest version package.json, pnpm-lock.yaml
  • fixes some types for the latest typescript compiler
    • add type coercion in MathtoolsTags.ts
    • explicitly ignores the u flag, which needs target >ES6 in HtmlMethods.ts and UnicodeConfiguration.ts
    • ensures promise type in Menu.ts (we could do this by coercing with as unknown as Promise<void> if you don't like the empty then.

Changes in Test Code

  • Updates packages to the latest version package.json, pnpm-lock.yaml
  • Moves jest configuration to a .mjs file as .ts now yields errors with latest typescript compiler
  • Update to tsconfig.js to include DOM library and update path for latest ts-jest version
  • Update type in setupTex.ts and include sources directly (as per our meeting).

Update to tests

Github Actions

  • Adds the test action in the .github/workflows. There were still a couple of things missing after our meeting, e.g., I had forgotten to compile the test helpers, the mjs files etc. I've ironed this all out now. As you can see tests are passing for this PR.
  • I added badges for tests and coverage to the README.

@zorkow zorkow changed the title Update/packages Update packages and add github actions for tests Jun 5, 2025
@zorkow zorkow requested a review from dpvc June 5, 2025 17:09
@zorkow zorkow marked this pull request as ready for review June 5, 2025 17:09
@zorkow
Copy link
Member Author

zorkow commented Jun 5, 2025

For the code coverage badge, we might have to reactivate code coverage at https://app.codecov.io/gh/mathjax/ using the admin account.

Copy link
Member

@dpvc dpvc left a comment

Choose a reason for hiding this comment

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

This all looks good. Thanks for doing it.

I am working on making the fonts use the scoped package, so that should make the temporary link for mathjax-full be unnecessary. I should have that shortly.

Also note that the current version of mathjax-newcm is 0.4.2-beta.8, but the package.json currently uses 0.4.0-beta.8. I don't think that will make any difference, but thought I should mention it anyway.

Comment on lines 38 to 43
pnpm mjs:compile
components/bin/makeAll --mjs --terse --build components/mjs
pnpm cjs:compile
pnpm cjs:components:src:build
components/bin/makeAll --cjs --terse --build components/cjs
pnpm copy:pkg cjs
Copy link
Member

Choose a reason for hiding this comment

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

Would you be willing to use pnpm -s rather than just pnpm for these (and the ones below)? That way, you only see that script output rather than the pnpm commands themselves. I think that looks cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

*/

import type {Config} from 'jest';
// import type {Config} from 'jest';
Copy link
Member

Choose a reason for hiding this comment

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

Remove this rather than comment out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I meant to do that and forgot.

@dpvc
Copy link
Member

dpvc commented Jun 5, 2025

We could adapt the file names of the test to the same spelling

What do you think is best? I don't have strong feelings either way.

@dpvc
Copy link
Member

dpvc commented Jun 5, 2025

I've made the v0.4.3-beta.8 releases of the fonts, so you might try switching to that and removing the mathjax-full link and see if that works now.

@codecov
Copy link

codecov bot commented Jun 5, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@zorkow
Copy link
Member Author

zorkow commented Jun 5, 2025

@dpvc I'll take care of your comments in a little while.
In the meantime: Here is our first successfully uploaded coverage test:
https://app.codecov.io/gh/mathjax/mathjax-src/tree/update%2Fpackages

@zorkow
Copy link
Member Author

zorkow commented Jun 5, 2025

We could adapt the file names of the test to the same spelling

What do you think is best? I don't have strong feelings either way.

I think it's good to be consistent. I will rename the files.

@zorkow
Copy link
Member Author

zorkow commented Jun 5, 2025

I've made the v0.4.3-beta.8 releases of the fonts, so you might try switching to that and removing the mathjax-full link and see if that works now.

I note that changing the font gets us back to the old values in Bussproofs.test.ts (e.g., 9.111em instead of 9.11em). Is that to be expected?

@zorkow
Copy link
Member Author

zorkow commented Jun 5, 2025

For the record:
Two more changes in tests node.test.ts and esm.test.ts that contained the mathjax-full path. That was hidden due to setting the link.

@zorkow zorkow merged commit 4775492 into develop Jun 5, 2025
3 checks passed
@zorkow zorkow deleted the update/packages branch June 5, 2025 21:28
@dpvc
Copy link
Member

dpvc commented Jun 5, 2025

Here is our first successfully uploaded coverage test:

That's pretty cool. Thanks for working that out.

I note that changing the font gets us back to the old values in Bussproofs.test.ts ... Is that to be expected?

Probably. I have been linking to the latest fonts, so the new values were probably in effect when I was writing all the TeX tests some months ago. I probably updated those tests at that time. But doing a pnpm install changed that back at some point, and that gave us the old values. You would be getting the same. Now that the 0.4.3-beta.8 fonts are available, they have the new values. I think I vaguely remember adjusting some spacing values, but don't remember the specifics.

Two more changes in tests node.test.ts and esm.test.ts that contained the mathjax-full path.

I was preparing a PR for that myself. The same change needs to be made in system.test.ts as well, though. (You didn't notice it because loading from a module fails in system.js, and so the failure was what was expected.

@dpvc
Copy link
Member

dpvc commented Jun 5, 2025

After this update, pnpm -s lint no longer runs. The eslint.config.mjs file imports from @eslint/js, but there is no longer an node_modules/@eslint directory if I remove node_modules and do a fresh pnpm install. If I change @eslint/js to eslint, then I get an error about the eslint.config.recommended (grepping for recommended in all the node_modules/eslint* directory finds nothing). Commenting out that configuration allows the linting to run, but I get warnings:

/Users/dpvc/Desktop/Folders/Tumlook/Data/Code/JavaScript/MathJax/Code/Git/mathjax/MathJax-src/ts/core/MmlTree/MmlNodes/mo.ts
  144:3  warning  Unused eslint-disable directive (no problems were reported from 'no-misleading-character-class')

/Users/dpvc/Desktop/Folders/Tumlook/Data/Code/JavaScript/MathJax/Code/Git/mathjax/MathJax-src/ts/input/tex/html/HtmlMethods.ts
  46:49  warning  Unused eslint-disable directive (no problems were reported from 'no-control-regex')

/Users/dpvc/Desktop/Folders/Tumlook/Data/Code/JavaScript/MathJax/Code/Git/mathjax/MathJax-src/ts/input/tex/mathtools/MathtoolsMappings.ts
  165:3  warning  Unused eslint-disable directive (no problems were reported from 'no-sparse-arrays')

/Users/dpvc/Desktop/Folders/Tumlook/Data/Code/JavaScript/MathJax/Code/Git/mathjax/MathJax-src/ts/input/tex/unicode/UnicodeConfiguration.ts
  137:7  error  Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free  @typescript-eslint/ban-ts-comment
  148:7  error  Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free  @typescript-eslint/ban-ts-comment

/Users/dpvc/Desktop/Folders/Tumlook/Data/Code/JavaScript/MathJax/Code/Git/mathjax/MathJax-src/ts/output/common/FontData.ts
  472:3  warning  Unused eslint-disable directive (no problems were reported from 'no-sparse-arrays')

✖ 6 problems (2 errors, 4 warnings)
  0 errors and 4 warnings potentially fixable with the `--fix` option.

@zorkow
Copy link
Member Author

zorkow commented Jun 6, 2025

After this update, pnpm -s lint no longer runs. The eslint.config.mjs file imports from @eslint/js, but there is no longer an node_modules/@eslint directory if I remove node_modules and do a fresh pnpm install. If I change @eslint/js to eslint, then I get an error about the eslint.config.recommended (grepping for recommended in all the node_modules/eslint* directory finds nothing). Commenting out that configuration allows the linting to run, but I get warnings:

There's always something... I did not try linting, I thought that was running automatically via husky.
What's odd is that updating the packages seems to have changed the eslint package source.
I'll have a look.

@dpvc dpvc added this to the v4.0 milestone Jun 17, 2025
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