mathjax / MathJax-node

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

[main] extensions options: support names containing "s" #402

Closed pkra closed 6 years ago

pkra commented 6 years ago

Fixes #401

pkra commented 6 years ago

Oh! I didn't push the actual commit. Oh well, might as well check the test for this part first.

pkra commented 6 years ago

The failing test is for the fontURL option. I think those tests should be expected to fail on old node/npm versions since the test relies on package-lock.json.

dpvc commented 6 years ago

Can the failing tests be bypassed in early version of node? (E.g., can you test for the node version and pass them if too early?)

dpvc commented 6 years ago

(Sorry, hit the wrong button).

pkra commented 6 years ago

Can the failing tests be bypassed in early version of node? (E.g., can you test for the node version and pass them if too early?)

Hm. We ran into this before, didn't we? I vaguely recall that the problem might resolve by updating the package-lock so as to have mathjax 2.7.4. I'll try that locally.

dpvc commented 6 years ago

I vaguely recall that the problem might resolve by updating the package-lock so as to have mathjax 2.7.4. I'll try that locally.

Maybe just have it look for any version in the string rather than look up the specific one. Or maybe you could load the mathjax module and get the version directly.

Something like

window = {};
window.MathJax = MathJax = {};
document = {getElementById: function () {}, childNodes: [], createElement: function () {}};
try {require("mathjax")} catch (e) {};
console.log(MathJax.version);

should get you the version of MathJax currently installed. It's a bit of a hack, but might work better than reading the package-lock file.

pkra commented 6 years ago

So we have three options

@dpvc and @zorkow, how would you want me to resolve this?

(Since I think the package-lock file should be updated to 2.7.4 for consistency, I'd favor this approach but it's not my call.)

dpvc commented 6 years ago

It seems to me that relying on a file (package-lock.json) that is not maintained in node 6 and 7 means that the test is broken. Even though the package-lock.json file has 2.7.3, if npm installs 2.7.4, it will update the version in the file automatically in versions of npm that understand the package-lock file (node 8 and above). Relying on package-lock in the other version is essentially the same as hard-coding the version number into the test file (and that would at least be more honest).

If we want to maintain tests for node 6 and 7, then I don't think we can rely on the package-lock version, since it need not be the actual version installed. If you want to run these tests in node 6 and 7, we need an alternative method of finding MthJax's version. I offered one above, but have worked out another one, which you can see in this comparison. Here, we use an extension that defines a TeX macro that returns the MathJax version, and then extract the version from the resulting output. The prevents having to polute the global namespace (as in my other approach).

This should work in all versions of node, and with whatever version of MathJax is loaded. That would be my recommendation. If that seems acceptable, I will make a separate pull request, and we can merge it prior to this one, and I think that will take care of the build fails without needing further modification to this PR.

pkra commented 6 years ago

Re https://github.com/mathjax/MathJax-node/pull/402#issuecomment-381288082

Copy that. I've made a PR #404 into this branch to get things started.

pkra commented 6 years ago

To recap, I've updated the test to fix the restart but I couldn't get the errors to hide from console.

I've also made a suggestion in the other PR to fix the test.

dpvc commented 6 years ago

Here is yet another way to get the version (and to get access to the MathJax object, which I hadn't thought was possible, but is).

mjAPI.typeset({
  math: '',
  format: "TeX",
  htmlNode: true,
}, function (data) {
  console.log(data.htmlNode.ownerDocument.defaultView.MathJax.version);
});

Perhaps that is better than the external module, since that doesn't require a configuration change, or external file?

pkra commented 6 years ago

Perhaps that is better than the external module, since that doesn't require a configuration change, or external file?

If you prefer this, I'm happy to update the test.

dpvc commented 6 years ago

I think merging the develop branch should now allow your PR to pass the tests. I've fixed a timing issue that was causing problems when tests were run sequentially even when they worked individually.

dpvc commented 6 years ago

The conflict listed in lib/main.js can be resolved by changing

MathJax.Hub.Queue(StartQueue);

to

MathJax.Hub.Queue(
    function () {sErrors = errors},
    StartQueue
);

in the develop version and removing the 401 version.

pkra commented 6 years ago

@dpvc could you review the conflict resolution?

dpvc commented 6 years ago

Looks good. Sorry for the long run around to get what in the end is a pretty small set of changes...