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

Allow to pass a custom KaTeX renderer #34

Closed
wants to merge 3 commits into from
Closed

Allow to pass a custom KaTeX renderer #34

wants to merge 3 commits into from

Conversation

StaloneLab
Copy link

Context

We needed to use the new mhchem plugin for a renderer, but were not able to do it using rehype-katex, because the KaTeX basically have no support for dynamic loading, so it would have been needed to "embed" the plugin directly into this plugin, which seems to me like a bad idea.

Resolution

I allow people to inject directly their KaTeX renderer inside of the plugin, so that they can load modules or anything.

Example

How to use it:

const katexWithPlugins = require("katex");
require("katex/dist/contrib/mhchem");

const unified = require("unified");
const remark = require("remark-parse");
const demath = require("remark-math");
const bridge = require("remark-rehype");
const sermath = require("rehype-katex");
const rehype = require("rehype-stringify");

unified()
	.use(remark)
	.use(demath)
	.use(bridge)
	.use(sermath, { katexRenderer: katexWithPlugins.renderToString })
	.use(rehype)
	.process("$\\ce{a A + b B -> c \\textbf{C} + d D}$", (e, vfile) => {
		console.log(String(vfile));
	});

What remains?

Review should be quite easy to do, but publishing the package would be necessary; I see that it was not published recently, so maybe you were working on something @wooorm ?

@codecov-io

This comment has been minimized.

@vhf
Copy link
Member

vhf commented Aug 30, 2019

Would it make sense to make katex a peer dependency here?

@StaloneLab
Copy link
Author

Tested this as I couldn't believe it would work... and it does!

Will update my PR to use peerDependency.

@vhf
Copy link
Member

vhf commented Aug 30, 2019

Just an idea I had that could make it a bit easier to configure rehype-math with a different katex renderer, I'm still not sure it really makes sense though.

(You'll have to do something like adding npm install katex in a before_script step in travis.yml for the CI to work.)

@StaloneLab
Copy link
Author

I think this is a good idea; devDeps are used in react for similar purposes and it seems to work: see react-dom that depends on react itself.

@wooorm
Copy link
Member

wooorm commented Aug 30, 2019

We need a test for this!

Also: why not create alternative packages? rehype-some-other-math-thing?

@vhf
Copy link
Member

vhf commented Aug 30, 2019

Also: why not create alternative packages? rehype-some-other-math-thing?

What rehype-katex currently does is apply a default katex, what this PR makes possible is for rehype-katex users to use it with katex + some katex extensions (the example given here adds mchem for instance).

We could create an alternative package but the best name I can think of would then be rehype-customizable-katex. I could be wrong but this tells me that making rehype-katex customizable would be more approriate. What do you think?

@StaloneLab
Copy link
Author

I'm not sure which kind of test would be good here: the one I added check plugins are loaded on remark-katex when they're loaded on katex.

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.

Huhh, KaTeX uses globals 😓 I didn’t know that, I thought this PR would be about passing a new function in, such as, where you’d do your own KaTeX processing or so.

The test should be different from what KaTeX normally would return, as it now uses the plugin. Maybe inline the result? Add a test with and without mhchem that are clearly different?

packages/rehype-katex/package.json Show resolved Hide resolved
@Rokt33r
Copy link
Member

Rokt33r commented Sep 2, 2019

Moving KaTeX to peerDeps makes sense to me. To test it properly, we need to install it into devDeps in root package.json too.

Also we need to udpate readme.md.

@StaloneLab
Copy link
Author

Huhh, KaTeX uses globals sweat I didn’t know that, I thought this PR would be about passing a new function in, such as, where you’d do your own KaTeX processing or so.

At first, I implemented it like that, but @vhf pointed out the peerDeps trick. I will write another test this evening and squash everything.

KaTeX is now both a dep and a peer-dep!

Sorry about that, I forgot to move it to devDeps, where it belongs, as @Rokt33r suggests.

@StaloneLab
Copy link
Author

Everything should be alright now, could you review @wooorm ?

Copy link
Member

@Rokt33r Rokt33r left a comment

Choose a reason for hiding this comment

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

And could you fix remark-html-katex too? It is a same package but for remark-html. :)

@@ -16,9 +16,13 @@
[npm][]:

```sh
npm install katex
Copy link
Member

Choose a reason for hiding this comment

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

I think npm install katex rehype-katex is better.

@wooorm
Copy link
Member

wooorm commented Sep 3, 2019

I’m still not really sure why these changes are needed. Extensions already work:

const unified = require('unified')
const markdown = require('remark-parse')
const remark2rehype = require('remark-rehype')
const html = require('rehype-stringify')
const math = require('remark-math')
const katex = require('rehype-katex')

require('katex/dist/contrib/mhchem')

unified()
  .use(markdown)
  .use(math)
  .use(remark2rehype)
  .use(katex)
  .use(html)
  .process('$\\ce{a A + b B -> c \\textbf{C} + d D}$', (e, vfile) => {
    console.log(String(vfile))
  })

Yields:

<p><span class="math math-inline"><span class="katex"><span class="katex-mathml"><math><semantics><mrow><mi>a</mi><mtext></mtext><mi mathvariant="normal">A</mi><mrow></mrow><mo>+</mo><mrow></mrow><mi>b</mi><mtext></mtext><mi mathvariant="normal">B</mi><mover><mo stretchy="true"></mo><mpadded width="+0.6em" lspace="0.3em"><mrow></mrow></mpadded></mover><mi mathvariant="normal">c</mi><mtext> </mtext><mtext mathvariant="bold">C</mtext><mrow></mrow><mo>+</mo><mrow></mrow><mi>d</mi><mtext></mtext><mi mathvariant="normal">D</mi></mrow><annotation encoding="application/x-tex">\ce{a A + b B -> c \textbf{C} + d D}</annotation></semantics></math></span><span class="katex-html" aria-hidden="true"><span class="base"><span class="strut" style="height:0.77777em;vertical-align:-0.08333em;"></span><span class="mord"><span class="mord mathdefault">a</span><span class="mspace" style="margin-right:0.16666666666666666em;"></span><span class="mord"><span class="mord mathrm">A</span></span><span class="mord"></span><span class="mspace" style="margin-right:0.2222222222222222em;"></span><span class="mbin">+</span><span class="mspace" style="margin-right:0.2222222222222222em;"></span><span class="mord"></span><span class="mord mathdefault">b</span><span class="mspace" style="margin-right:0.16666666666666666em;"></span><span class="mord"><span class="mord mathrm">B</span></span><span class="mspace" style="margin-right:0.2777777777777778em;"></span><span class="mrel x-arrow"><span class="vlist-t vlist-t2"><span class="vlist-r"><span class="vlist" style="height:0.622em;"><span style="top:-3.144em;"><span class="pstrut" style="height:2.5220000000000002em;"></span><span class="sizing reset-size6 size3 mtight x-arrow-pad"><span class="mord mtight"></span></span></span><span class="svg-align" style="top:-2.511em;"><span class="pstrut" style="height:2.5220000000000002em;"></span><span class="hide-tail" style="height:0.522em;min-width:1.469em;"><svg width="400em" height="0.522em" viewBox="0 0 400000 522" preserveAspectRatio="xMaxYMin slice"><path d="M0 241v40h399891c-47.3 35.3-84 78-110 128
-16.7 32-27.7 63.7-33 95 0 1.3-.2 2.7-.5 4-.3 1.3-.5 2.3-.5 3 0 7.3 6.7 11 20
 11 8 0 13.2-.8 15.5-2.5 2.3-1.7 4.2-5.5 5.5-11.5 2-13.3 5.7-27 11-41 14.7-44.7
 39-84.5 73-119.5s73.7-60.2 119-75.5c6-2 9-5.7 9-11s-3-9-9-11c-45.3-15.3-85
-40.5-119-75.5s-58.3-74.8-73-119.5c-4.7-14-8.3-27.3-11-40-1.3-6.7-3.2-10.8-5.5
-12.5-2.3-1.7-7.5-2.5-15.5-2.5-14 0-21 3.7-21 11 0 2 2 10.3 6 25 20.7 83.3 67
 151.7 139 205zm0 0v40h399900v-40z"></path></svg></span></span></span><span class="vlist-s"></span></span><span class="vlist-r"><span class="vlist" style="height:0.01100000000000001em;"><span></span></span></span></span></span><span class="mspace" style="margin-right:0.2777777777777778em;"></span><span class="mord"><span class="mord mathrm">c</span></span><span class="mspace nobreak"> </span><span class="mord text"><span class="mord textbf">C</span></span><span class="mord"></span><span class="mspace" style="margin-right:0.2222222222222222em;"></span><span class="mbin">+</span><span class="mspace" style="margin-right:0.2222222222222222em;"></span><span class="mord"></span><span class="mord mathdefault">d</span><span class="mspace" style="margin-right:0.16666666666666666em;"></span><span class="mord"><span class="mord mathrm">D</span></span></span></span></span></span></span></p>
Without the extension
<p><span class="math math-inline"><span class="katex"><span class="katex-mathml"><math><semantics><mrow><mstyle mathcolor="#cc0000"><mtext>\ce</mtext></mstyle><mrow><mi>a</mi><mi>A</mi><mo>+</mo><mi>b</mi><mi>B</mi><mo></mo><mo>></mo><mi>c</mi><mtext mathvariant="bold">C</mtext><mo>+</mo><mi>d</mi><mi>D</mi></mrow></mrow><annotation encoding="application/x-tex">\ce{a A + b B -> c \textbf{C} + d D}</annotation></semantics></math></span><span class="katex-html" aria-hidden="true"><span class="base"><span class="strut" style="height:1em;vertical-align:-0.25em;"></span><span class="mord text" style="color:#cc0000;"><span class="mord" style="color:#cc0000;">\ce</span></span><span class="mord"><span class="mord mathdefault">a</span><span class="mord mathdefault">A</span><span class="mspace" style="margin-right:0.2222222222222222em;"></span><span class="mbin">+</span><span class="mspace" style="margin-right:0.2222222222222222em;"></span><span class="mord mathdefault">b</span><span class="mord mathdefault" style="margin-right:0.05017em;">B</span><span class="mord"></span><span class="mspace" style="margin-right:0.2777777777777778em;"></span><span class="mrel">></span><span class="mspace" style="margin-right:0.2777777777777778em;"></span><span class="mord mathdefault">c</span><span class="mord text"><span class="mord textbf">C</span></span><span class="mspace" style="margin-right:0.2222222222222222em;"></span><span class="mbin">+</span><span class="mspace" style="margin-right:0.2222222222222222em;"></span><span class="mord mathdefault">d</span><span class="mord mathdefault" style="margin-right:0.02778em;">D</span></span></span></span></span></span></p>

@StaloneLab
Copy link
Author

I don't know how you did, but I did not succeed, and it's exactly why this PR exist: to avoid uncertainty in the process. I do not know how NPM works internally, so couldn't tell why it sometimes works and some other times don't... Maybe @vhf have a clue on this, but anyway, it adds no cost making it a peerDep.

@@ -2,4 +2,5 @@ language: node_js
node_js:
- lts/dubnium
- node
before_script: npm install katex
Copy link
Member

Choose a reason for hiding this comment

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

katex will be installed without extra install scripts because it is already in devdeps.

@wooorm
Copy link
Member

wooorm commented Sep 4, 2019

I don't know how you did, but I did not succeed, and it's exactly why this PR exist: to avoid uncertainty in the process. I do not know how NPM works internally, so couldn't tell why it sometimes works and some other times don't... Maybe @vhf have a clue on this, but anyway, it adds no cost making it a peerDep.
— @TitiAlone

how NPM works internally

npm would typically install katex directly into your-project/node_modules, if you also use rehype-katex.

In some cases there can be both a your-project/node_modules/rehype-katex/node_modules/katex and a your-project/node_modules/katex, if your project and rehype-katex depend on different versions of KaTeX, but even then, loading the plugin from your-project/node_modules/katex/dist/contrib/mhchem would probably work fine.
This would also happen if we’re going with this PR and you install katex directly.

How npm works is, well, a long story. I’d need to know more about what specifically your setup is.

it adds no cost making it a peerDep

It does, unfortunately, typically not work: peer dependencies are not normally installed, so the default of this plugin would be broken.

Peer dependencies are used for plugins neighbouring their host tool. For example, in remark-math we can define a peer dependency of remark — because we don’t include remark in remark-math, but remark-math does not work with all versions of remark.
In rehype-katex, we are importing katex. Having katex as a peer dependency would mostly break rehype-katex.

I don’t understand how moving this to a peer dependency fixed your problem. I don’t think it’s broken.

More info:

@Rokt33r
Copy link
Member

Rokt33r commented Sep 5, 2019

I agree with @wooorm 's opinion. And we've done same thing in rehype-highlight and other similar libraries too. So I guess we should just update documentation rather than moving katex to peerDeps.

@wooorm
Copy link
Member

wooorm commented Sep 5, 2019

@vhf What are your thoughts on this?

@vhf
Copy link
Member

vhf commented Sep 5, 2019

I'm a bit confused to be honest.

I never actually attempted to use katex extensions with rehype-katex. If what you describe here works, I don't think we should proceed with this PR. If under certain circumstances it doesn't, I'd be interested in an example.

Regarding deps vs peer deps I don't have anything to add. :)

@StaloneLab
Copy link
Author

I will try this soon and tell you if I'm able to reproduce my problem. Will close this PR if it everything works.

@StaloneLab
Copy link
Author

Tested it right now on my laptop and I can't get it work, procedure as follow:

  • create a new npm project: npm init on a new directory, everything left as default;
  • install everything needed: npm install unified katex remark-parse remark-math remark-rehype rehype-katex rehype-stringify
  • create an index.js file with the content below;
  • run it: node index.js.

I get the following error: 3:3: KaTeX parse error: Undefined control sequence: \ce at position 1: \̲c̲e̲{a A + b B -> c
And mhchem elements are not being renderer.

Content of my index.js:

const unified = require("unified");
const remark = require("remark-parse");
const math = require("remark-math");
const remark2rehype = require("remark-rehype");
const katex = require("rehype-katex");
const rehype = require("rehype-stringify");

require("katex/dist/contrib/mhchem");

const input = `# Maths\n\n$$\\ce{a A + b B -> c \\textbf{C} + d D}$$`;

unified()
    .use(remark)
    .use(math)
    .use(remark2rehype)
    .use(katex)
    .use(rehype)
    .process(input, (e, data) => {
        if(e) console.error(e);
        console.log(data);
    })

@wooorm
Copy link
Member

wooorm commented Sep 6, 2019

@TitiAlone Thanks for checking!

You can make it work by a) using rehype-katex/node_modules/katex/dist/contrib/mhchem in this case, or b) using katex@^0.10.0 in the project.
If we publish the (many, and breaking) changes in remark-math, it would work by default again.

@StaloneLab
Copy link
Author

Should I close this issue, then?

@wooorm
Copy link
Member

wooorm commented Sep 8, 2019

Sorry for the wait, yes I’ll close this, and publish the majors!

@wooorm wooorm closed this Sep 8, 2019
@wooorm wooorm added 🙋 no/question This does not need any changes 🙅 no/wontfix This is not (enough of) an issue for this project labels Sep 8, 2019
@MrWillCom MrWillCom mentioned this pull request Feb 17, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 no/question This does not need any changes 🙅 no/wontfix This is not (enough of) an issue for this project
Development

Successfully merging this pull request may close these issues.

5 participants