mathjax / MathJax-node

MathJax for Node
Apache License 2.0
615 stars 97 forks source link

should mmlNode:true force mml:true? #277

Closed pkra closed 7 years ago

pkra commented 7 years ago

mmlNode requires mml but not vice versa (whereas it's the reverse for the other outputs).

pkra commented 7 years ago

What's worse (as compared to the other outputs) is that mmlNode:true actually fails to produce anything unless mml:true is set.

dpvc commented 7 years ago

It looks like these two lines should be

    var mml = jax.root.toMathML('',jax);
    if (data.mml) result.mml = mml;
    if (data.mmlNode) result.mmlNode = jsdom(mml).body.firstChild;

instead.

On looking at this closer, there may be some other problems. For example, the result.mmlNode will have parent node still attached to the jsdom document's body, so you may want to use removeChild() to give a detached node instead (I haven't looked at the other Node results to see if they have a similar problem.

pkra commented 7 years ago

Thanks! I'll make a PR.

dpvc commented 7 years ago

Actually, it looks like you may need to use

    if (data.mml || data.semantic || data.speechText) result.mml = mml;

since result.mml is needed for those functions. But since you will be removing those features, you may want to handle it differently. If you do ignore those features, then it looks like line 538 should just be

  if (!data.mml && !data.mmlNode) return;
pkra commented 7 years ago

Yes, that's what's in the open PR, cf. https://github.com/mathjax/MathJax-node/blob/e4c32b99b494ccb18bd43d86a0bdfda6d11e1076/lib/main.js#L514

pkra commented 7 years ago

PS: I'd prefer to wait with a new PR until the other one has been merged.

dpvc commented 7 years ago

Ok. The real error at this point is the addition of if (data.mml) to line 541. It should have been left as

    result.mml = jax.root.toMathML('',jax);

since result.mml is used in several other locations, and is eventually removed in line 779 if it is not requested by the user. I didn't catch this in review.

pkra commented 7 years ago

=> Ready for Review.

dpvc commented 7 years ago

I think there may have been a misunderstanding, here (and the PR was merged before I was able to respond to it). My original comments were intended to say that result.mml should not be produced when data.mmlNode is true (unless data.mml is also true). My final comment above indicates that the removal of if (data.mml) in line 541 would be offset by the deletion of result.mml in line 779. But the latter line was removed in the previous pull request (removing SRE), so that compensation is no longer occurring.

That means result.mml should now only be set when data.mml is true, and the earlier approach from my initial comment is now the right one.

In any case, result.mml should only be produced when data.mml is present, as was the case in the past, and as is consistent with svgNode and htmlNode.

I made a branch that does this (see below), and will make a pull request for it.