[plugin system] Add defineSymbol to the main katex object #1263

HosseinAgha · 2018-04-19T13:59:20Z

Discussed in #762
I did not add anything to contribution guidelines as this is still in beta but I think it is ready to be merged.
I will also add a plugin template to the project when the API is finalized.
Please check out the PluginManager class as it contains the main logic. Then you can check out a KaTeX plugin in action: persian-katex-plugin.
Here are some things that I was not sure about or I needed your guidance to implement:

KaTeX uses closures to store its state (functions, symbols, etc). This makes the plugins changes permanent (you cannot reset the library to default). A good approach that I can think of is to use classes to keep these data structures but this will break the main KaTeX API as users have to instantiate the KaTeX class in order to use it. So I did not touch the old system.
I could not find a clean way to expose the KaTeX flow types to plugins. Do you have any suggestions?
For plugins to dynamically inject their new css classes I could think of 2 solutions:
- Migrate KaTeX styles to a css in js solution like aphrodite
- Ask user to add plugins css in addition to KaTeX.css.
  I used this approach as I wasn't sure if it is OK to change the old behaviour.
I think migrating KaTeX development server and github.io website to a React App like the one used in persian-katex-plugin opens up whole new possibilities e.g. we can have buttons to apply different plugins to the library and check out their effect. I'm ready to implement that if you are OK.
I also added yarn.lock as there is no harm for it to be there plus it provides faster and safer installation for people who use yarn and most known libraries usually have it in addition to package-lock.json: just removed yarn.lock from this PR
persian-katex-plugin adds new symbols, fonts and fontMetrics so it only uses defineSymbol and addFontMetrics. I'm not sure if we need to expose anything more than defineFunction to other plugins as I haven't used it yet: we will not expose defineFunction in this PR

All the current tests are passing.
I will also add more tests for the plugin system (and squash the commits) when we finalize the API.

khanbot · 2018-04-19T13:59:22Z

CLA signature looks good 👍

codecov · 2018-04-19T14:03:40Z

Codecov Report

Merging #1263 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1263    /-   ##
=======================================
  Coverage   82.44%   82.44%           
=======================================
  Files          60       60           
  Lines        3880     3880           
  Branches      651      651           
=======================================
  Hits         3199     3199           
  Misses        578      578           
  Partials      103      103

Impacted Files	Coverage Δ
katex.js	`73.91% <ø> (ø)`	⬆️
src/symbols.js	`100% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4b57cb...2fe22d2. Read the comment docs.

ylemkimon · 2018-04-19T14:33:49Z

Thank you for the pull request! Could you separate code style fixes to another PR?

HosseinAgha · 2018-04-20T10:27:14Z

@ylemkimon I dropped the cleanup commit

kevinbarabash

It seems like there are still lots of formatting changes. I think this could (should?) be split into two separate PRs. One for exposing defineSymbol and one for adding and exposing addFontMetrics.

kevinbarabash · 2018-04-21T22:48:17Z

src/PluginManager.js

+const compiler = {
+ defineSymbol,
+ defineFunction,
+ addFontMetrics,


Why not just expose these functions directly on the katex object? There isn't really a concept of plugins internally. I think defineSymbol is relatively stable, but defineFunction requires access to a lot of internal classes to be useful.

These functions are basically internal utilities of KaTeX. I thought having an additional abstraction layer in API makes room for future breaking changes without breaking the API.
I did not think exposing them to normal users was a good idea on the other hand advanced users can use them without defining a new javascript class (plugin) so I'll directly expose them as you're OK with it.

We can expose them with a __ underscore prefix to indicate that they should be used with caution.

Yes I refactored the code.
Please checkout my following comment.

HosseinAgha · 2018-04-22T06:42:31Z

@kevinbarabash

I reverted all remaining cleanups
Added defineSymbol to main katex object only in this PR
Removed old PluginManager class and katex.plugin function
Removed the yarn.lock from the PR
Created a new PR for changes related to addFontMetric: katex #1269
Rebased and squashed all the commits
Abandon exposing defineFunction altogether as it has to be done in another PR

kevinbarabash · 2018-04-22T17:14:45Z

katex.js

+ /**
+ * adds a new symbol to internal symbols table
+ */
+ __defineSymbol: defineSymbol,


kevinbarabash · 2018-04-22T17:17:55Z

src/symbols.js

@@ -19,7 19,7 @@

 import type {Mode} from "./types";

-type Font = "main" | "ams";
+type Font = string; // can be "main" | "ams" | any other injected font by plugins


I'm uncertain that any value that isn't "main" or "ams" would actually work correctly. If we allow any string, there should be a test in katex-spec.js that verifies that other values work.

It will not work until user defines a font-face and adds appropriate font metrics to use a new font.
I will revert this as we have to deal with this issue in #1269. These types are not much useful for katex users until we add katex libdef to flow-typed project or use something like .js.flow files when they got stable.
After deleting this I don't think we need any new tests in this PR.

edemaine · 2018-04-22T18:31:14Z

I'm not sure if this is the PR for it, but it occurs to me that exposing defineMacro (for global macros, as an alternative to passing them in) could also be handy.

kevinbarabash · 2018-04-22T22:25:05Z

Exposing defineMacro as __defineMacro could be a separate PR.

This PR serves as a complement to PR KaTeX#1232 by supporting some letters that are omitted from the Unicode range 1D400–1D7FF.

* Include Bold-Italic fonts for \boldsymbol Fix KaTeX#1228 * Update screenshots

* README: Add size badge Add a size badge showing size of gzipped katex.min.js * update-sri: Add code to replace size badge in readme * Change to use badge from shields.io instead My bad for assuming that badgesize.io supports https. Change to use the badge from shields.io which supports https. * More Unicode letters (KaTeX#1260) This PR serves as a complement to PR KaTeX#1232 by supporting some letters that are omitted from the Unicode range 1D400–1D7FF. * Include Bold-Italic fonts for \boldsymbol (KaTeX#1257) * Include Bold-Italic fonts for \boldsymbol Fix KaTeX#1228 * Update screenshots * README: Add size badge Add a size badge showing size of gzipped katex.min.js * update-sri: Add code to replace size badge in readme * Change to use badge from shields.io instead My bad for assuming that badgesize.io supports https. Change to use the badge from shields.io which supports https. * Use badgesize.io with https

* Fix space width in \texttt Fix KaTeX#1255 via KaTeX/katex-fonts#41. Also fix width metrics via KaTeX/katex-fonts#42. * Improve test * Update screenshots

HosseinAgha · 2018-04-24T06:22:25Z

@kevinbarabash everything is fixed, I think we are ready to merge.

kevinbarabash · 2018-04-28T19:43:45Z

src/symbols.js

@@ -19,7 19,7 @@

 import type {Mode} from "./types";

-type Font = "main" | "ams";
+type Font = "main" | "ams"


I'm surprised that this passed the linter.

kevinbarabash

LGTM

kevinbarabash · 2018-06-06T03:27:33Z

I could not find a clean way to expose the KaTeX flow types to plugins. Do you have any suggestions?

I need to read up on this more, but it sounds like you can define an index.js.flow file.

For plugins to dynamically inject their new css classes I could think of 2 solutions:

Migrate KaTeX styles to a css in js solution like aphrodite

Ask user to add plugins css in addition to KaTeX.css. I used this approach as I wasn't sure if it is OK to change the old behaviour.

My hope is that we can reduce the need for CSS in general in the future. It limits our ability to support other rendering backends in the future like canvas or SVG. Obviously with the case of adding support for additional fonts some CSS has to be defined somewhere in order to define new font faces. I think what you did is fine.

I think migrating KaTeX development server and github.io website to a React App like the one used in persian-katex-plugin opens up whole new possibilities e.g. we can have buttons to apply different plugins to the library and check out their effect. I'm ready to implement that if you are OK.

Being able to try out different plugins would be awesome. I'd try making that work first in the dev server environment. One thing I'd like to investigate before introducing React for the test app is look at restructuring this repo to be a mono-repo. See #1411 for details about moving to a mono-repo.

HosseinAgha requested review from kevinbarabash and edemaine April 19, 2018 14:01

edemaine removed request for edemaine and kevinbarabash April 19, 2018 14:52

HosseinAgha force-pushed the master branch from 4f4650b to c7e0d23 Compare April 20, 2018 08:22

kevinbarabash reviewed Apr 21, 2018

View reviewed changes

HosseinAgha force-pushed the master branch 4 times, most recently from d64faca to 53f31c8 Compare April 22, 2018 06:35

HosseinAgha changed the title ~~KaTeX Plugin System~~ Add defineSymbol to the main katex object [plugin system] Apr 22, 2018

kevinbarabash reviewed Apr 22, 2018

View reviewed changes

ronkok and others added 5 commits April 23, 2018 10:43

More Unicode letters (KaTeX#1260)

47986ab

This PR serves as a complement to PR KaTeX#1232 by supporting some letters that are omitted from the Unicode range 1D400–1D7FF.

Include Bold-Italic fonts for \boldsymbol (KaTeX#1257)

cf89b88

* Include Bold-Italic fonts for \boldsymbol Fix KaTeX#1228 * Update screenshots

Fix space width in \texttt (KaTeX#1261)

1686b28

* Fix space width in \texttt Fix KaTeX#1255 via KaTeX/katex-fonts#41. Also fix width metrics via KaTeX/katex-fonts#42. * Improve test * Update screenshots

Expose defineSymbol in the main KaTeX object

697196e

HosseinAgha force-pushed the master branch from a8aef6f to 697196e Compare April 23, 2018 06:13

Merge branch 'master' into master

40f4eda

HosseinAgha changed the title ~~Add defineSymbol to the main katex object [plugin system]~~ [plugin system] Add defineSymbol to the main katex object Apr 25, 2018

kevinbarabash reviewed Apr 28, 2018

View reviewed changes

Merge branch 'master' into master

2fe22d2

kevinbarabash approved these changes Apr 28, 2018

View reviewed changes

kevinbarabash merged commit bd7a977 into KaTeX:master Apr 28, 2018

ylemkimon mentioned this pull request Aug 18, 2018

Release v0.10.0-rc #1634

Merged

1 task

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

RunasSudo mentioned this pull request Sep 24, 2022

Supporting Custom Fonts #1010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[plugin system] Add defineSymbol to the main katex object #1263

[plugin system] Add defineSymbol to the main katex object #1263

HosseinAgha commented Apr 19, 2018 •

edited

Loading

khanbot commented Apr 19, 2018

codecov bot commented Apr 19, 2018 •

edited

Loading

ylemkimon commented Apr 19, 2018

HosseinAgha commented Apr 20, 2018

kevinbarabash left a comment

kevinbarabash Apr 21, 2018

HosseinAgha Apr 22, 2018 •

edited

Loading

kevinbarabash Apr 22, 2018

HosseinAgha Apr 22, 2018 •

edited

Loading

HosseinAgha commented Apr 22, 2018 •

edited

Loading

kevinbarabash Apr 22, 2018

kevinbarabash Apr 22, 2018

HosseinAgha Apr 23, 2018 •

edited

Loading

edemaine commented Apr 22, 2018

kevinbarabash commented Apr 22, 2018

HosseinAgha commented Apr 24, 2018

kevinbarabash Apr 28, 2018

kevinbarabash left a comment

kevinbarabash commented Jun 6, 2018 •

edited

Loading

[plugin system] Add defineSymbol to the main katex object #1263

[plugin system] Add defineSymbol to the main katex object #1263

Conversation

HosseinAgha commented Apr 19, 2018 • edited Loading

khanbot commented Apr 19, 2018

codecov bot commented Apr 19, 2018 • edited Loading

Codecov Report

ylemkimon commented Apr 19, 2018

HosseinAgha commented Apr 20, 2018

kevinbarabash left a comment

Choose a reason for hiding this comment

kevinbarabash Apr 21, 2018

Choose a reason for hiding this comment

HosseinAgha Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

kevinbarabash Apr 22, 2018

Choose a reason for hiding this comment

HosseinAgha Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

HosseinAgha commented Apr 22, 2018 • edited Loading

kevinbarabash Apr 22, 2018

Choose a reason for hiding this comment

kevinbarabash Apr 22, 2018

Choose a reason for hiding this comment

HosseinAgha Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

edemaine commented Apr 22, 2018

kevinbarabash commented Apr 22, 2018

HosseinAgha commented Apr 24, 2018

kevinbarabash Apr 28, 2018

Choose a reason for hiding this comment

kevinbarabash left a comment

Choose a reason for hiding this comment

kevinbarabash commented Jun 6, 2018 • edited Loading

HosseinAgha commented Apr 19, 2018 •

edited

Loading

codecov bot commented Apr 19, 2018 •

edited

Loading

HosseinAgha Apr 22, 2018 •

edited

Loading

HosseinAgha Apr 22, 2018 •

edited

Loading

HosseinAgha commented Apr 22, 2018 •

edited

Loading

HosseinAgha Apr 23, 2018 •

edited

Loading

kevinbarabash commented Jun 6, 2018 •

edited

Loading