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

Adds KaTeX math engine via the 'katex' ruby gem #489

Merged
merged 1 commit into from Feb 3, 2018

Conversation

glebm
Copy link
Contributor

@glebm glebm commented Jan 25, 2018

This is a simpler all batteries included alternative to the sskatex engine.

@ccorn
Copy link
Contributor

ccorn commented Jan 25, 2018

I have merged the proposed fix by @glebm to SsKaTeX and made a new release (no longer needs to overwrite EXECJS_RUNTIME to defer autodetection). Re-triggering the CI tests should now load the katex gem normally.

@glebm
Copy link
Contributor Author

glebm commented Jan 25, 2018

I've updated the PR to trigger a test re-run

@glebm glebm force-pushed the katex branch 2 times, most recently from 50dc1b4 to dbb46ba Compare January 25, 2018 03:23
@glebm
Copy link
Contributor Author

glebm commented Jan 25, 2018

The pre-2.3 targets are failing because the katex gem requires ruby 2.3+.

@gettalong Perhaps kramdown could drop official support for older rubies by removing them from .travis.yml? Ruby 2.2 EOL is scheduled for 2018-03-31.

@ccorn
Copy link
Contributor

ccorn commented Jan 25, 2018

You may have to constrain the katex gem version in .travis.yml so that the availability of a version with a newer katex.js (and different HTML output) does not automatically trigger CI failures.

@ccorn
Copy link
Contributor

ccorn commented Jan 25, 2018

For earlier rubies, the load failure would be reflected in the AVAILABLE member. You just need to exclude the installation attempts and the new test cases when the engine is not available.

  • You could replace the katex gem installation in .travis.yml with:

    - ruby -e 'exit(RUBY_VERSION < "2.3" ? 0 : 1)' || gem install katex some_version_constraint_here
    
  • and augment test/test_files.rb with KATEX_AVAILABLE guards like SsKaTeX does.

@glebm glebm force-pushed the katex branch 2 times, most recently from b2c6794 to 3e73fb1 Compare January 25, 2018 19:41
@glebm
Copy link
Contributor Author

glebm commented Jan 25, 2018

Thanks @ccorn, I've done as you suggested and Travis is now green

@gettalong
Copy link
Owner

So @glebm, could you explain to me what this implementation offers over sskatex?

@glebm
Copy link
Contributor Author

glebm commented Jan 25, 2018

@gettalong The katex gem is much easier to set up, because it bundles all the KaTeX assets and registers them automatically with all the popular Ruby web frameworks (Rails, Sprockets without Rails, and Hanami).

See also the discussion at ccorn/sskatex#1

@ccorn
Copy link
Contributor

ccorn commented Jan 26, 2018

From my point of view:

  • The katex engine is a locked-down fixed-configuration engine for untrusted users:
    • No Javascript fragments configurable, thus suitable for applications which need to disable all means of code injection
    • JS engine determined by ExecJS autodetection, not overridable
    • Needs to cache only one execution context (within the gem) because there is only one configuration available
    • KaTeX version determined by gem version; no macros nor other options that SsKaTeX's katex_opts would allow to be passed to KaTeX math_engine_opts passed directly to KaTeX; throwOnError turned off by default
    • No logging
  • SsKaTeX is for trusted users who need more control or stability:
    • KaTeX version is controlled by the user directly
    • Passes KaTeX options via katex_opts, throws on errors by default
    • Can change all JS engine and code aspects, supports logging/debug messages
    • May be able to interface with MathJax by using different js_libs
    • Makes sure to pass ASCII strings only
      • Useful for ancient engines like SpiderMonkey 1.8.5 which can run KaTeX just fine but may have been compiled without support for UTF-8 I/O
      • Note that such engines are deprecated in ExecJS and not autoselected.

Let me add that KaTeX version control is important; e.g. in recent releases the handling of \color has changed (which is intentional but backwards-incompatible by default).

@ccorn
Copy link
Contributor

ccorn commented Jan 26, 2018

In Rakefile, the added line s.add_development_dependency 'katex' may need to be conditioned on RUBY_VERSION as well. Edit: Rather not. rake test works, and rake dev:gem shall not be affected.

Anyway, to avoid repeating that criterion too often, the KATEX_AVAILABLE test in test/test_files.rb could be made similar to the one for MATHJAX_NODE_AVAILABLE (that is, just test the AVAILABLE member). This would then work regardless of whether katex is a development dependency or not.

@gettalong
Copy link
Owner

@glebm If it is only that, then there is no real reason to add this to kramdown. If there were any big features that katex has but sskatex hasn't, than it would make sense. But from kramdown's point of view it doesn't make sense to provide two ways to do effectively the same.

@ccorn
Copy link
Contributor

ccorn commented Jan 26, 2018

The workings and outputs are indeed almost the same (if the underlying KaTeX versions are the same), almost because SsKaTeX escapes non-ASCII characters passing to and from the engine.

But the target user sets are disjoint: untrusted users on a github-pages-like service vs trusted individuals with their own specific setups. It solves the security issue discussed in #471 by bundling all the ingredients and binding the associated maintenance costs to the katex gem. I imagine that the katex version could be advertised for general use, and if individuals run into problems with their personal setups, they can be asked to use SsKaTeX for better control.

(I fear that some cloud service provider with unsecured kramdown setup will come complaining that his platform spent most of its computing power in horribly inefficient bitcoin mining. In that case, it would be good to be able to say not only RTFM but that there is a secure KaTeX alternative.)

@gettalong gettalong self-assigned this Jan 27, 2018
Copy link
Owner

@gettalong gettalong left a comment

Choose a reason for hiding this comment

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

@glebm I have thought about this PR and decided to accept it. Some needed changes can be seen in this review.

Thanks for your contributions! And @ccorn - thanks for your input!

.travis.yml Outdated
@@ -22,6 +22,8 @@ before_install:
- sudo apt-get install texlive-latex-base texlive-latex-recommended texlive-fonts-recommended texlive-latex-extra tidy
- gem install rake
- gem install minitest coderay stringex ritex execjs sskatex
- >
ruby -e 'exit RUBY_VERSION < "2.3" ? 0 : 1' || gem install katex -v 0.3.0
Copy link
Owner

Choose a reason for hiding this comment

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

Please put this on one line, there is no need for splitting it across two.

Copy link
Contributor Author

@glebm glebm Jan 30, 2018

Choose a reason for hiding this comment

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

It contains a :. Do you know how it should be escaped if it's on one line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured it out, done.

Copy link
Owner

Choose a reason for hiding this comment

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

Hm... always something new to learn 👍 You are correct, the colon poses a problem, sorry. So leave that as it is.

@@ -0,0 +1,29 @@
---
Copy link
Owner

Choose a reason for hiding this comment

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

Please reformat the documentation to 100 columns. And you also need to link to this page on the documentation.page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -50,7 +50,7 @@ module Converter

configurable(:math_engine)

['Mathjax', "MathjaxNode", "SsKaTeX", "Ritex", "Itex2MML"].each do |klass_name|
%w(Mathjax MathjaxNode Katex SsKaTeX Ritex Itex2MML).each do |klass_name|
Copy link
Owner

Choose a reason for hiding this comment

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

Please just add your mathe engine here. No need to change the styling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It then does not fit on one line. How do you want it indented if it's split across multiple lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, from looking at other code I see that the undocumented column limit is not 80.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Owner

Choose a reason for hiding this comment

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

Column limit is 100. When I have some more time I will add support for a linting tool...


def self.call(converter, el, opts)
display_mode = el.options[:category] == :block
result = ::Katex.render el.value,
Copy link
Owner

Choose a reason for hiding this comment

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

This method call is missing parentheses.

Copy link
Contributor Author

@glebm glebm Jan 30, 2018

Choose a reason for hiding this comment

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

Parenthesis are optional here. Do you have a style guide / a lint command to make sure I haven't missed anything else?

Copy link
Owner

Choose a reason for hiding this comment

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

No, not for kramdown. I would just use them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -42,14 +42,16 @@ class TestFiles < Minitest::Test
warn("Skipping MathjaxNode tests as MathjaxNode is not available")
end

KATEX_AVAILABLE = begin
SSKATEX_AVAILABLE = begin
Copy link
Owner

Choose a reason for hiding this comment

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

The following lines should be indented two additional spaces since SSKATEX_AVAILABLE is two characters longer than KATEX_AVAILABLE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ccorn
Copy link
Contributor

ccorn commented Jan 31, 2018

  • Adding KaTeX to the Output list in doc/documentation.page makes no sense.
  • What requires updates are doc/converter/html.page (math section) and doc/documentation.template (math engines).
  • In test/test_files.rb, line 46 (below SSKATEX_AVAILABLE) still requires +2 indent.

@ccorn
Copy link
Contributor

ccorn commented Jan 31, 2018

The sources doc/converter/html.page and/or the individual math engine's pages should probably include a discussion katex vs sskatex:

katex sskatex
easy to use more control
for untrusted users for trusted users

I'd provide one in a follow-up PR if you do not want to touch SsKaTeX parts. In two rough sentences: Use katex unless you need more control and can be trusted to configure sskatex. Do not make the sskatex gem available to a service with untrusted users.

@glebm
Copy link
Contributor Author

glebm commented Jan 31, 2018

@ccorn I've addressed your comments.

I'd provide one in a follow-up PR if you do not want to touch SsKaTeX parts.

I'm going to take you up on this, thank you.

@ccorn
Copy link
Contributor

ccorn commented Jan 31, 2018

In doc/math_engine/katex.page, it should IMHO be mentioned where the fonts and CSS are stored, at least with a link to the gem's manual registration section so that e.g. Jekyll users can configure their system appropriately.

@glebm
Copy link
Contributor Author

glebm commented Jan 31, 2018

@ccorn I've added a link to the Assets section in the gem's docs to avoid duplicating any specifics here, because they may get out of date. For example, as Jekyll is a common use case, I'd like to support Automatic Registration for Jekyll in the future as well, if possible.

Aside: if you use jekyll-assets, then automatic registration works (Sprockets without Rails case).

@ccorn
Copy link
Contributor

ccorn commented Jan 31, 2018

Just mentioning that there are no Rdoc comments in lib/kramdwon/converter/math_engine/katex.rb. But there is really not much to say anyway, and the adjacent itex2mml.rb or ritex.rb (which have very similar structure) are about as sparse.

@ccorn
Copy link
Contributor

ccorn commented Jan 31, 2018

IMHO, this PR (checked up to 4216c67) now has all required ingredients.
(I'd move the KaTeX in the documentation.template closer to SsKaTeX, leaving the default MathJax at the top of the list, but that's just my taste.)
I have also tried it on a small Jekyll tree with a couple hundred formulas, and it works (after setting the KaTeX assets to those of v0.8.3; I sometimes try other snapshots).

@gettalong
Copy link
Owner

@glebm Thanks for all the work - please squash the commits into one and rebase on current master, then I will merge.

Thanks for your contribution!

This is a simpler all batteries included alternative to the `sskatex` engine.
@glebm
Copy link
Contributor Author

glebm commented Feb 3, 2018

@gettalong Done!

@gettalong gettalong merged commit 7e53f8b into gettalong:master Feb 3, 2018
@gettalong
Copy link
Owner

@glebm Thanks - I have merged the commit.

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.

None yet

3 participants