mathjax / MathJax-node

MathJax for Node
Apache License 2.0
614 stars 96 forks source link

[main] extensions options breaks with 2+ extensions #401

Closed pkra closed 6 years ago

pkra commented 6 years ago

This seems to be due to

https://github.com/mathjax/MathJax-node/blob/a3c71820b5c0cf0848a48d86df9828aed9c796d2/lib/main.js#L531

missing an escape -- I suspect it should be var extensionList = extensions.split(/\s*,\s*/);.

pkra commented 6 years ago

Note for later: the documentation should mention that it expects a comma separated string of extension paths. (Perhaps the option could also accept an array of strings at some point.)

pkra commented 6 years ago

While working around this (by using the MathJax configuration block), I realized that it's no longer a convenience option in so far as the new support for third party extensions is restricted to this option, i.e., it's not possible to specify a custom path and use that in the MathJax block. Not sure if that's fixable or not but another reason to fix this.

pkra commented 6 years ago

Ah, it seems that this workaround is not working either; I'm guessing the extensions option overwrites the parts from the MathJax block. I'll take a closer look when I get around to fixing this issue.

dpvc commented 6 years ago

I'm sorry, I don't understand what you are saying, here. You can't include extensions in your MathJax block, as that overrides the extensions set up within mathjax-node itself. But if you include "toMathJax.js", that should work, since it is the only one included there. Similarly for TeX: {extensions: [...]}, since mathjax-node sets that. If you include it in your configuration, it will lose the ones mathjax-node sets, so you would have to include them as well.

Note the extensions string will work with more than one extension, as long as the extension doesn't end in an s. In particular, don't use the .js at the end (as mathjax-node will add that, if needed). So extensions: "TeX/color,[contrib]/myextension" should work.

pkra commented 6 years ago

D'oh! Thanks for clarifying. I confused myself there.

dpvc commented 6 years ago

No problem. So other than fixing the missing backslash, is there anything else that you still consider to be an error?

pkra commented 6 years ago

So other than fixing the missing backslash, is there anything else that you still consider to be an error?

Nothing else. Thanks again.

dpvc commented 6 years ago

OK, great. Thanks!

pkra commented 6 years ago

I have trouble catching the error. The errors do not seem to be added to result.errors.

Here's the test I had in mind:

var tape = require('tape');
var mjAPI = require("../lib/main.js");

tape('MathJax configuration: allow multiple extensions', function (t) {
    t.plan(1);
    mjAPI.config({
        extensions: "TeX/autoload-all.js, TeX/color.js"
    });
    mjAPI.typeset({
        math: 'E = mc^2',
        format: "TeX",
        mml: true,
    }, function (data) {
        t.notOk(data.errors, 'Config block with multiple extensions throws no error');
    });
});
pkra commented 6 years ago

(PS: The console logs an error from jsdom's resource loader.)

dpvc commented 6 years ago

OK, this is what is happening: the file load error is coming during the MathJax startup process, which occurs before the typeset action the you request is performed. But errors are cleared when a typeset operation is started, so that means the startup errors are lost when your typeset is started, and so you don't get them in the returned results.

It is a small change to the library to retain the startup errors. Here's the diffs:

diff --git a/lib/main.js b/lib/main.js
index aac703e..5a23ff9 100644
--- a/lib/main.js
+++ b/lib/main.js
@@ -91,6 +91,7 @@ var document, window, content, html; // the DOM elements
 var queue = [];       // queue of typesetting requests of the form [data,callback]
 var data, callback, originalData;   // the current queue item
 var errors = [];      // errors collected durring the typesetting
+var sErrors = [];     // errors collected durring MathJax startup
 var ID = 0;           // id for this SVG element

 //
@@ -519,7 +520,10 @@ function ConfigureMathJax() {
         if (MathJax.OutputJax.SVG.resetGlyphs) MathJax.OutputJax.SVG.resetGlyphs(true);
         MathJax.ElementJax.mml.ID = 0;
         serverState = STATE.READY;
-        MathJax.Hub.Queue(StartQueue);
+        MathJax.Hub.Queue(
+          function () {sErrors = errors}, 
+          StartQueue
+        );
       });
     }
   };
@@ -730,7 +734,7 @@ function GetSVG(result) {
 //
 function StartQueue() {
   data = callback = originalData = null;       //  clear existing equation, if any
-  errors = [];                  //  clear any errors
+  errors = sErrors; sErrors = [];              //  clear any errors
   if (!queue.length) return;    //  return if nothing to do

   serverState = STATE.BUSY;

If you are preparing a PR, perhaps you can include these changes as well. Otherwise, I can make the PR for them.

pkra commented 6 years ago

Thanks, Davide. I'll add them to my pr.