yishn / tikzcd-editor

A simple visual editor for creating commutative diagrams.
https://tikzcd.yichuanshen.de/
MIT License
1.86k stars 100 forks source link

Conversion to MathJax 3 #53

Closed cemulate closed 3 years ago

cemulate commented 3 years ago

As discussed in this issue, a move to MathJax 3 is likely to fix the offending obscure MathJax bug.

This was a minor ordeal, but I took the plunge and did it, with some relatively thorough testing to ensure feature parity.

Besides the obvious API changes, there are some major structural changes to the code that I'll outline here for your reviewing convenience:

Loading and Configuration

The story here is quite different. Now the main bundle file does the following as the first JS that runs:

This was the best solution I could come up with to resolve loading order difficulties (the config object must exist before the library is loaded, etc.), but you may have better ideas here

Ensure the library is loaded before using the API

The other trouble was with components using the MathJax api before it was ready (particularly apparent when loading from a hash url); I'm much less confident in my solution here since I'm much more of a Vue guy, so I'm not sure if my solution is idomatic in a react-like environment:

I separated this part into a separate commit in case you have any better ideas on how to tackle this. In general, loading the library asynchronously and enforcing the library's readiness as a precondition is a huge pain in cases like these.


I also have the current build hosted at https://cemulate.github.io/tikzcd-editor, if you'd like to do any testing yourself. Here is a non-trivial hash url to use.

yishn commented 3 years ago

Thanks a lot for your work!

This was the best solution I could come up with to resolve loading order difficulties (the config object must exist before the library is loaded, etc.), but you may have better ideas here

It looks fine.

The other trouble was with components using the MathJax api before it was ready (particularly apparent when loading from a hash url); I'm much less confident in my solution here since I'm much more of a Vue guy, so I'm not sure if my solution is idomatic in a react-like environment

I'm also fine with rendering only when MathJax is ready (so there will be no need for a mathjaxLoaded prop on App). That will be easier to reason about. There can potentially be a flash of blank page but it shouldn't be much of a problem.

yishn commented 3 years ago

Another thing I've noticed by playing with your build is that panning the given nontrivial diagram isn't as smoothly as before. It seems MathJax is forcing a reflow on mouse move. I'm not sure why it didn't do that previously... maybe previously the MathJax queue deferred all updates to a single actual update?

Performance

cemulate commented 3 years ago

Interesting. When I started investigating that it seems that I get the same behavior on the tip of master, and it's even noticeable on an empty diagram (but obviously moreso with some TeX). Yet, I get great performance on your hosted site at http://tikzcd.yichuanshen.de/. I did a reasonable amount of double checking here to make sure I wasn't crazy, but I can definitely reproduce this behavior on a fresh build from master.

Is it perhaps the case that the production site was built from an older commit? If so, we could bisect and maybe find the culprit.

yishn commented 3 years ago

Hmm, I can reproduce it locally on master as well. The only change that has been done since last release was to update the dependencies. Maybe the performance issue can be tackled separately.

I would still like you to remove the mathjaxLoaded prop on App though, so App will only be rendered when MathJax is completely ready.

cemulate commented 3 years ago

Cool, I rebased on master and in the process replaced the second commit (resolving the mathjax ready issue) with a new approach that just renders the main component when MathJax is ready. There's a gap of "blank page" at load but very small.

I'm curious about the performance issue. It seems to be that panning the Grid causes all GridCells to re-render (because Grid re-renders). So I don't know how this issue wasn't present before, but perhaps a preact upgrade either fixed or introduced a bug that's making the behavior different.

EDIT: Not sure why the build is failing; I made sure to run format in the most recent commit, and npm run build reports no Prettier issues.

cemulate commented 3 years ago

Just FYI on the panning performance issue: I removed the typesetting completely and still got the same lag, so it seems to be a performance issue with preact. In particular, when panning, there is are periodic moments a second or so apart where preact re-renders all grid cells at once, and this is when a noticable freeze/lag happens.

yishn commented 3 years ago

Thanks a lot for your investigation! The build issue comes from a new prettier version. Just perform a npm i and npm run format to format all files again and you should be fine.

I agree, the lag most likely comes from the preact update.

cemulate commented 3 years ago

No problem! I took care of the formatting.