fix bug in level - the code increases the level by 1 but doesn't reduce it by 1 #42
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uce it by -1 This bug might not be so evident if you're just using the html renderer of markdown-it. However, I use markdown-it to parse markdown and create an AST that is then manipulated using slatejs, and to some subtle use of caching is needed to make this performant. If you look at the similar plugin https://github.com/iktakahiro/markdown-it-katex/blob/master/index.js and also https://github.com/markdown-it/markdown-it-container/blob/master/index.js you'll see that the third argument that you're pushing onto the state change the level, and the code here doesn't change the level back. This means that the rest of the document gets parsed with the level being at least 1 too large. I think you should instead just pass 0 (as is done for math_inline), since there's only one token that gets emitted, rather than an open/close pair. Thanks. I hope this is helpful to somobody. I wish github itself used a proper parser like this one for math in Markdown, since what they are currently doing isn't so good: https://news.ycombinator.com/item?id=31450597
williamstein
changed the title
|
Thanks for detecting and fixing this bug. |
zampino
added a commit
to nextjournal/markdown
that referenced
this pull request
Nov 28, 2023
bug fixed upstream with goessner/markdown-it-texmath#42
zampino
added a commit
to nextjournal/markdown
that referenced
this pull request
Nov 28, 2023
The formula plugin we're using didn't restore the parser level after pushing their tokens, this messed up our criterium for adding nodes into the table of contents. The bug was fixed upstream in v1.0.0 with goessner/markdown-it-texmath#42.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This bug might not be so evident if you're just using the html renderer of markdown-it. However, I use markdown-it to parse markdown and create an AST that is then manipulated using slatejs, and to some subtle use of caching is needed to make this performant. If you look at the similar plugin
https://github.com/iktakahiro/markdown-it-katex/blob/master/index.js
and also
https://github.com/markdown-it/markdown-it-container/blob/master/index.js
you'll see that the third argument that you're pushing onto the state change the level, and the code here doesn't change the level back. This means that the rest of the document gets parsed with the level being at least 1 too large. I think you should instead just pass 0 (as is done for math_inline), since there's only one token that gets emitted, rather than an open/close pair.
Thanks. I hope this is helpful to somobody. I wish github itself used a proper parser like this one for math in Markdown, since what they are currently doing isn't so good: https://news.ycombinator.com/item?id=31450597