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

rehype-mathjax: allow configuration of the TeX input processor #54

Merged
merged 9 commits into from
Feb 5, 2021

Conversation

MartyO256
Copy link
Contributor

Closes GH-53

I extend the options type of the non-browser rehype-mathjax plugin to enable configuring the TeX input processor in a non-breaking way.
I kept the modifications to a minimum, but restructuring of the plugin may be required in the future.
Extending the options this way has a number of consequences:

  • The type for the options now has format OutputOptions & {tex: TexInputOptions} which is different from that of MathJax. A breaking change will be required in the future to make the options type one of {tex: TexInputOptions, svg: SvgOutputOptions } or {tex: TexInputOptions, chtml: ChtmlOutputOptions}.
  • Since MathJax throws when unsupported options keys are supplied to the various processors, a hack is introduced to remove the added tex key from the output options.
  • When automatic equation numbering is used, the transformer would throw when processing multiple documents using the same rehype instance. That is because instances of the MathJax document may not be reused across processed documents in this case. Creation of the renderer is moved from the attacher to the transformer.
  • Validation to ensure that a fontURL is provided in the CHTML output processor would take place in the attacher from the createRenderer function. Because that creation was moved to the transformer, a hack is introduced as an optional parameter to the createPlugin function to perform the validation in the attacher without adding imports.
  • The createRenderer function would create a JSDOM adaptor and register a HTML handler in MathJax. This would take place in the attacher (through the createRenderer function), which is appropriate since rehype-mathjax does not allow configuring the handler (to add the AssistiveMmlHandler as in this demo for instance). The adaptor creation and handler registration have been moved outside the renderer function to prevent a new handler to be registered for each processed document. This handler is never unregistered. In the future, registering of the handlers should be moved to the beginning of the transformer, and the handlers should be unregistered at the end of the transformer.

MathJax throws if unsupported keys are added to the SVG and CHTML options.
Hence, if TeX options are supplied, the output options are shallow copied and the 'tex' key is deleted.
An error is expected to be thrown from the attacher if `fontURL` is missing from the CHTML output processor options.
Previously, this error originated from the `createRenderer` function, so the error-throwing statement had to be moved.
Handlers are registered to a singleton instance of MathJax.
Since the creation of the renderer has been moved from the attacher to the transformer, the previous implementation would register a new HTML handler for each document processed and never unregister it.
All documents may use the same adaptor (and registered handler) because no configuration is currently supported for the handlers.
Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Great work, thanks! Some nits inline.

One more q: could you maybe add some of your points in the PR body inline so that we won’t forget them?
E.g.,

// To do next major: find a way to unregiste adapters

@@ -2,19 +2,25 @@ const visit = require('unist-util-visit')

module.exports = createPlugin

function createPlugin(displayName, createRenderer) {
function createPlugin(displayName, createRenderer, chtml = false) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function createPlugin(displayName, createRenderer, chtml = false) {
function createPlugin(displayName, createRenderer, chtml) {

Default of undefined is fine as well and supported everywhere

function createInput() {
return new Tex({packages: packages})
function createInput(options) {
return new Tex({packages: packages, ...options})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return new Tex({packages: packages, ...options})
return new Tex(Object.assign({packages: packages}, options))

Copy link
Member

Choose a reason for hiding this comment

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

(But also see next comment)

delete settings.tex
}

return new Svg(settings)
Copy link
Member

Choose a reason for hiding this comment

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

Can these changes in output-svg and output-chtml be done higher: can lib/core.js make parse and compile options instead?

And: does mathjax support unknown keys when they‘re set to undefined?

E.g. I’m thinking:

var inputOptions = settings.tex
var outputOptions = Object.assign({}, settings)
// either of these two lines:
outputOptions.tex = undefined
delete outputOptions.tex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MathJax would still throw with outputOptions.tex = undefined.
Moving the options format adjustment to the attacher required another flag (browser) to the createPlugin function because the options for the browser plugin correspond to the TeX input options, and not output options as in the non-browser plugins.

@wooorm wooorm added 🗄 area/interface This affects the public interface 🙆 yes/confirmed This is confirmed and ready to be worked on 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change labels Jan 12, 2021
The code that would remove the `tex` options from the output options was duplicated in the `output-chtml` and `output-svg` files.
It was moved to the attacher instead, and the signature of the `createRenderer` function was adjusted to reflect the separation of the input and output options.
MathJax does not support `tex: undefined` in the output options, so `delete` has to be used.

The options for the browser plugin correspond to the TeX input processor.
This would conflict with the previous hack used for the non-browser plugin.
Hence, a `browser` flag was added to the `createPlugin` function.
@MartyO256 MartyO256 requested a review from wooorm January 20, 2021 16:58
@wooorm
Copy link
Member

wooorm commented Jan 20, 2021

thanks for the ping and your patience, I’ll get to it soon.
Also /cc @tani who first wrote this subpackage, who might have ideas on it!

/* To do next major: Remove `chtml` and `browser` flags once all the options use
the same format */

function createPlugin(displayName, createRenderer, chtml, browser) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You use two new arguments chtml browser to declare what feature is enabled, don't you?
If so, I prefer to use features argument like

// Typescript: 
// type Feature = "chtml" | "browser"
// type Features = Array<Feature>
function createPlugin(displayName, createRenderer, features) {
// blah blah blah ...
  if (features.includes("chtml") && ...)

I feel the above code is simpler than.

Copy link
Contributor Author

@MartyO256 MartyO256 Jan 20, 2021

Choose a reason for hiding this comment

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

I agree that a single string flag parameter could be used. However, the flags I have currently do not correspond to supported features. Rather, they are only used internally to know how to "normalize" the input format, without increasing the bundle size.

https://github.com/remarkjs/remark-math/pull/54/files#diff-58df6482f24a55ac377b4e8eed3a71947c97309172516b6c10f0b647dc54f8f3R20-R25

That is, I rearrange the options based on the current client interface, whereby

  • The options for the browser plugin are those of the TeX input processor,
  • The options for the non-browser plugins are those of the corresponding output processor (CHTML or SVG).

This PR extends the options with the key tex to support configuring the TeX input processor for the non-browser plugins.
Ultimately, the flags and normalization should be removed when the options format is changed to match that of MathJax in the next major version.

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @MartyO256!

Comment on lines +19 to +37
// http://docs.mathjax.org/en/latest/options/input/tex.html#the-configuration-block
interface MathJaxInputTexOptions {
packages: string[]
inlineMath: [[string, string]]
displayMath: [[string, string]]
processEscapes: boolean
processEnvironments: boolean
processRefs: boolean
digits: RegExp
tags: 'none' | 'ams' | 'all'
tagSide: 'left' | 'right'
tagIndent: string
useLabelIds: boolean
multlineWidth: string
maxMacros: number
maxBuffer: number
baseURL: string
formatError: (jax: any, err: any) => string
}
Copy link
Member

Choose a reason for hiding this comment

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

do the Math Jax typings have an interface for this that could be reused?
Could these easily be upstreamed to MathJax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't a typing that could be reused. I doubt that this could be upstreamed to MathJax because of its component architecture. Internally, MathJax uses type OptionList = {[name: string]: any} (from Options.ts) pretty much everywhere for configuration, probably because the actual type for the options is determined dynamically.

There is validation in MathJax that throws for unhandled keys in the options (for instance, it throws if tex is a key for the CHTML output processor). I don't know specifically where that validation takes place, but it seems to me like a proper type for the options cannot be inferred statically. Inferring the type would require merging the various option types read from the list of packages configured in the input processors. That could be done if there were some schema object for the options of each component, in a similar fashion as how type inferrence works with Yup.

@wooorm wooorm merged commit 39070ab into remarkjs:main Feb 5, 2021
@wooorm wooorm added ⛵️ status/released and removed 🙆 yes/confirmed This is confirmed and ready to be worked on labels Feb 5, 2021
@wooorm
Copy link
Member

wooorm commented Feb 5, 2021

Thanks for your patience, @MartyO256. Released!

@wooorm wooorm added the 💪 phase/solved Post is done label Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

rehype-mathjax: allow configuration of TeX input processor
4 participants