sonoisa / XyJax-v3

Xy-pic extension for MathJax version 3
Apache License 2.0
34 stars 3 forks source link

[README] Add instructions for server-side rendering #5

Closed artagnon closed 1 year ago

artagnon commented 3 years ago

This addresses #4. We should probably patch xypic to work with liteDOM instead of requiring JSDOM.

dpvc commented 3 years ago

@sonoisa, I would not recommend accepting this PR, as the example code is flawed in several ways. One is that it doesn't include the custom jsdom adaptor that it references. Another is that it mixes the use of MathJax components with direct module imports, which is not a supported model. Another is that it uses the MathJax.loader inefficiently.

The reason he is using jsdom here is that your code includes a few places where you assume the SVG elements are actual browser DOM elements, which fails with the LiteDOM that is used for server-side processing. These few places need to be converted to use the DOM adaptor, as you have in most other locations. Here is a diff that shows the needed changes.

diff --git a/src/output/CHTMLWrappers.js b/src/output/CHTMLWrappers.js
index 6d8706a..546d895 100644
--- a/src/output/CHTMLWrappers.js
+++ b/src/output/CHTMLWrappers.js
@@ -66,9 +66,9 @@ augment(Shape.TextShape, {
                        thisWrapper.toCHTML(thisRoot);

                        const origin = svg.getOrigin();
-                       thisRoot.setAttribute("data-x", (c.x - halfW - origin.x + p * scale));
-                       thisRoot.setAttribute("data-y", (-c.y - halfHD - origin.y + p * scale));
-                       thisRoot.setAttribute("data-xypic-id", this.math.xypicTextObjectId);
+                       adaptor.setAttribute(thisRoot, "data-x", (c.x - halfW - origin.x + p * scale));
+                       adaptor.setAttribute(thisRoot, "data-y", (-c.y - halfHD - origin.y + p * scale));
+                       adaptor.setAttribute(thisRoot, "data-xypic-id", this.math.xypicTextObjectId);
                        parent.appendTextObject(thisRoot);

                        // for DEBUGGING
@@ -275,8 +275,8 @@ export class CHTMLxypic extends AbstractCHTMLxypic {
                                adaptor.setStyle(chtml, "vertical-align", round2(- box.d - p + MathJax.xypic.measure.axis_height) + "em");

                                for (let to of this._textObjects) {
-                                       const tx = parseFloat(to.getAttribute("data-x"));
-                                       const ty = parseFloat(to.getAttribute("data-y"));
+                                       const tx = parseFloat(adaptor.getAttribute(to, "data-x"));
+                                       const ty = parseFloat(adaptor.getAttribute(to, "data-y"));
                                        adaptor.setStyle(to, "left", "" + round2(tx - xOffsetEm) + "em");
                                        adaptor.setStyle(to, "top", "" + round2(ty - yOffsetEm) + "em");
                                }
diff --git a/src/output/Graphics.js b/src/output/Graphics.js
index 1d9bb4e..1cb4cf2 100644
--- a/src/output/Graphics.js
+++ b/src/output/Graphics.js
@@ -64,12 +64,12 @@ class SVG {
                                }
                        }
                }
-               this.drawArea.appendChild(obj);
+               this.appendChild(obj);
                return obj;
        }

        appendChild(svgElement) {
-               this.drawArea.appendChild(svgElement);
+               this.xypicWrapper.adaptor.append(this.drawArea, svgElement);
                return svgElement;
        }

These changes allow it work with the LiteDOM (at least with the examples that I tried). It would be best not to have to rely on jsdom, as it is very slow in comparison to LiteDOM.

See the discussion at mathjax/MathJax-demos-node#32 to see the evolution of the example that appears in this PR. There are some other examples there that might be more appropriate (once the changes above are made).

I haven't had the chance to go over your code in detail, but I will be making some suggests when I can.

artagnon commented 3 years ago

These changes allow it work with the LiteDOM (at least with the examples that I tried)

I found the example where it fails: try

\begin{xy}
\xymatrix{
\color{red}{\textbf{FORTRAN (1957)}}^{*}\ar[dr] & \textbf{LISP (1958)}\ar[dl]\ar[dddl]\ar[dddr] & \textbf{ALGOL (1958)}\ar[r]\ar[dl]\ar[drr] & \textbf{Simula (1962)}\ar[dlll]\ar@/_4pc/[ddddd]\ar@/^1pc/[dddll] \\
\texttt{Smalltalk (1972)}\ar@{-->}@/_2pc/[ddddrr]\ar@{-->}@/^1.6pc/[ddddrrrr]^>>>>>>>>>>>>>>>>>>>>>>>>>{\texttt{Self (1987)}} & \texttt{C (1972)}^{*}\ar@{.>}[dd]\ar@{.>}[ddrr]\ar@{.>}[ddddl]\ar@{.>}@/_3pc/[ddddddrrr] & \textbf{Prolog (1972)}\ar[dd] & \textbf{ML (1973)}\ar@{~~>}@/_2pc/[ddddll]\ar@{~~>}@/^2pc/[ddddrr] & \color{red}{\texttt{Bourne Shell (1978)}}\ar[ddll] \\
\\
\texttt{Common Lisp (1984)}\ar@/_2pc/[ddddrr] & \color{green}{\texttt{C++ (1985)}}^{*}\ar[ddrr]\ar[ddddddl] & \texttt{Erlang (1986)}^{*}\ar@/_2pc/[ddddll] & \texttt{Perl (1987)}\ar[ddlll]\ar[ddl] & \color{green}{\textbf{Coq (1989)}}\ar@{~>}@/_0.5pc/[ddddlll]\ar@{~>}[ddddl] \\
\\
\texttt{Python (1990)} & \color{brown}{\texttt{Haskell (1990)}}\ar[dd]\ar[ddl]\ar[ddrr] & \color{brown}{\texttt{Ruby (1995)}} & \color{red}{\texttt{Java (1995)}}^{*}\ar[ddl]|{*}\ar[ddlll]|{*} & \color{red}{\texttt{Javascript (1995)}}^{*} & \texttt{OCaml (1996)}\ar@/^0.1pc/[ddddlllll] \\
\\
\color{brown}{\texttt{Scala (2004)}} & \texttt{Agda (2007)}\ar@/_2pc/[rr] & \color{brown}{\texttt{Clojure (2007)}} & \color{blue}{\texttt{Idris (2007)}} & \color{red}{\texttt{Go (2009)}} \\
\\
\texttt{Rust (2010)}
}
\end{xy}

Here's the stack trace:

[object Object]: TypeError: Cannot read property 'parent' of undefined
TypeError: Cannot set property 'parent' of null
    at LiteAdaptor.replace (/Users/artagnon/src/artagnon.com/node_modules/mathjax-full/js/adaptors/liteAdaptor.js:248:26)
    at HTMLMathItem.updateDocument (/Users/artagnon/src/artagnon.com/node_modules/mathjax-full/js/handlers/html/HTMLMathItem.js:44:34)
    at HTMLDocument.AbstractMathDocument.updateDocument (/Users/artagnon/src/artagnon.com/node_modules/mathjax-full/js/core/MathDocument.js:453:26)
    at HTMLDocument.updateDocument (/Users/artagnon/src/artagnon.com/node_modules/mathjax-full/js/handlers/html/HTMLDocument.js:174:45)
    at Object.renderDoc (/Users/artagnon/src/artagnon.com/node_modules/mathjax-full/js/core/MathDocument.js:111:63)
    at RenderList.renderDoc (/Users/artagnon/src/artagnon.com/node_modules/mathjax-full/js/core/MathDocument.js:122:35)
    at HTMLDocument.AbstractMathDocument.render (/Users/artagnon/src/artagnon.com/node_modules/mathjax-full/js/core/MathDocument.js:282:28)
    at HTMLDocument.AbstractMathDocument.rerender (/Users/artagnon/src/artagnon.com/node_modules/mathjax-full/js/core/MathDocument.js:288:14)
    at /Users/artagnon/src/artagnon.com/lib/mathjax.js:39:22
    at Array.forEach (<anonymous>)

Looks like a bug in LiteAdaptor, but I can't be sure.

dpvc commented 3 years ago

It works for me (using LiteDOM), both in my original code, and in yours in the PR (when I remove the jsdom adaptor and make custom point to xypic/build:

xypic
artagnon commented 3 years ago

You're absolutely right; I'd not built xypic correctly. It works perfectly now! See #6.

artagnon commented 3 years ago

Okay, this is ready to merge as soon as #6 is merged.

artagnon commented 3 years ago

Thanks for the review. I've addressed it appropriately.