shime / livedown

Live Markdown previews for your favorite editor.
MIT License
765 stars 51 forks source link

Fix GitHub-links rendering #49

Closed hcgatewood closed 3 years ago

hcgatewood commented 5 years ago

NOTE. Due to my misunderstanding of how GitHub pull requests work, this pull requests currently includes multiple updates:

For that final update (functionality across files), the zombie tests are not working as I would expect them to--e.g., setting a timeout is having no effect, and the tests are still failing despite visual confirmation. If you have any thoughts, would be much appreciated!

Anyways, let me know when you've taken a look at this @shime and then we can delineate the updates (for now, I'm using my forked version of the repo locally, so for convenience I'll leave this as-is until you're ready to accept the pull request). /NOTE.

Identified, fixed, and tested a bug that prevented effectively utilizing a table of contents.

Bug

The GitHub-flavored generation of intra-document links is broken.

The markdown-it-github-headings tracks how many of the same link it has seen. However, the current version does not refresh these numbers across chokidar's change-driven reloads. This results in undesirable postfixes on unique links--and the postfixes change across document updates! This breaks all attempts to provide e.g. a table of contents in Markdown files rendered by Livedown.

For example. Before, we can see that the h3 "Lifting" has the id "lifting-3"; after document update, "Lifting" now has the id "lifting-4". This pattern will repeat ad infinitum. before after

Fix

Create a new renderer object on every detected document update. This resets the link counts.

Tests

All tests pass locally. Render time change is negligible (anecdotally +/- 5%).

shime commented 5 years ago

Thanks for your hard work on this, @hcgatewood.

Didn't notice the issue with heading's number increasing on every change. Thanks for reporting.

Unfortunately, the fix you are proposing is too hacky for me. Can we come up with something better? Looks like markdown-it-github-headings suggests some workarounds for dealing with link prefixes. Maybe we can look there for inspiration?

Thanks for adding KaTeX support, I'm good with merging that part in.

hcgatewood commented 5 years ago

For sure, thanks for checking out pull requests!

Excited for the KaTeX pull, I'll set it up as a separate PR in a bit.

For the links issue, here's why I think the solution in this PR is the "correct" solution given the constraints.

"Correct" output. That is, the HTML id's currently generated by Livedown are not accurate to what are seen on GitHub. This is really only relevant though for people who may be hand-inspecting the Livedown-rendered HTML document, so nbd.

The bug is outside our control. The real problem is that markdown-it is not upholding its implicit contract as a rendering engine--namely, that its output should be a partial function (meaning a particular input maps only and always to its associated single output). Whether we blame markdown-it for allowing plugins to break this contract, or markdown-it-github-headings itself for breaking the contract, is outside Livedown's control. Thus, since the first-best solution (a correctly-behaving rendering engine--or, at least, a rendering engine library that exports a function to initialize a new instance of the engine without having to re-import the library) is currently unattainable, we need a second-best solution.

Short-term workarounds. In my experience, when concocting a short-term workaround, we want something that is both effective and, importantly, simple. Simplicity is desired to make it easier to transition to the first-best solution--that is, we want to make it simple and easy to upgrade to a compliant version of markdown-it-github-headings. That said, I don't know that adding client-side JavaScript to solve this problem is less hacky, and I definitely feel it's less simple.

Would love your thoughts!