Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Code and heading after list without blank line #2483

Merged
merged 4 commits into from
Jun 13, 2022

Conversation

kiyoka
Copy link
Contributor

@kiyoka kiyoka commented May 28, 2022

fixed behavior of fenced code block following list.
fixed behavior of heading following list.

Marked version:
4.0.16

Markdown flavor:
GitHub Flavored Markdown

Description

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR)
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

fix: behavior of fenced code block following list.
fix: behavior of heading following list.
@vercel
Copy link

vercel bot commented May 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
marked-website ✅ Ready (Inspect) Visit Preview Jun 12, 2022 at 1:48PM (UTC)

@calculuschild
Copy link
Contributor

calculuschild commented May 28, 2022

While this PR does work, at this point it's starting to look like we are going to end up replicating the entirety of the "Paragraph" interruption RegEx that we already have in rules.js. I don't think the Specs were clear on this, but now it is becoming clear that we should be interrupting list items with the same rules as paragraph interruptions, even though "tight" lists don't end up rendering a <p> tag.

For instance, we should also be interrupting list items with "headers", "blockquotes", and "HTML" which we don't do correctly if we compare to the commonmark Dingus: Marked Demo, Commonmark Demo

There must be some way we can just incorporate the existing interruption regex here rather than re-implementing each one, which results in code duplication and a lot of complexity to the Lists tokenizer. @UziTech thoughts?

@UziTech
Copy link
Member

UziTech commented May 28, 2022

I agree. I think we should apply similar logic as the paragraph. It might still have to be duplicated since it won't be able to break on lists like the paragraph.

@@ -0,0 +1,7 @@
<ol>
<li>abcd<pre><code>if {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is not supposed to be part of the list item. CommonMark demo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UziTech
Thanks for the review.
My misunderstanding.
I thought that Marked's behavior of code blocks was aimed at a different behavior than CommonMark.

I will fix it and commit to it.
I will conform to CommonMark's behavior as follows.

Note: The CommonMark demo site cannot keep spaces in permalinks, so I have attached two images.

Followed the behavior of CommonMark.js.
@kiyoka
Copy link
Contributor Author

kiyoka commented Jun 10, 2022

@UziTech
I have corrected the pointed out issues.
Please review again.

src/Tokenizer.js Outdated
@@ -223,6 +223,15 @@ export class Tokenizer {
endEarly = true;
}

const fencesBeginRegExp = new RegExp(`^( {0,${indent}})(\`\`\`|~~~)`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the 2nd if statement if this only checked {0, ${indent - 1}}?

src/Tokenizer.js Outdated
Comment on lines 227 to 237
if (fencesBeginRegExp.test(src)) { // Items begin with at most one code block
const fenceBeginCap = fencesBeginRegExp.exec(src);
// if End list item if found non-indented fenced code block
if (fenceBeginCap[1].length < indent) {
endEarly = true;
}
}

if (!endEarly) {
const nextBulletRegex = new RegExp(`^ {0,${Math.min(3, indent - 1)}}(?:[*+-]|\\d{1,9}[.)])((?: [^\\n]*)?(?:\\n|$))`);
const hrRegex = new RegExp(`^ {0,${Math.min(3, indent - 1)}}((?:- *){3,}|(?:_ *){3,}|(?:\\* *){3,})(?:\\n+|$)`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (fencesBeginRegExp.test(src)) { // Items begin with at most one code block
const fenceBeginCap = fencesBeginRegExp.exec(src);
// if End list item if found non-indented fenced code block
if (fenceBeginCap[1].length < indent) {
endEarly = true;
}
}
if (!endEarly) {
const nextBulletRegex = new RegExp(`^ {0,${Math.min(3, indent - 1)}}(?:[*+-]|\\d{1,9}[.)])((?: [^\\n]*)?(?:\\n|$))`);
const hrRegex = new RegExp(`^ {0,${Math.min(3, indent - 1)}}((?:- *){3,}|(?:_ *){3,}|(?:\\* *){3,})(?:\\n+|$)`);
if (!endEarly) {
const nextBulletRegex = new RegExp(`^ {0,${Math.min(3, indent - 1)}}(?:[*+-]|\\d{1,9}[.)])((?: [^\\n]*)?(?:\\n|$))`);
const hrRegex = new RegExp(`^ {0,${Math.min(3, indent - 1)}}((?:- *){3,}|(?:_ *){3,}|(?:\\* *){3,})(?:\\n+|$)`);
const fencesBeginRegex = new RegExp(`^( {0,${Math.min(3, indent - 1)}})(\`\`\`|~~~)`);

Then below you can do a simpler test:

            // End list item if found code fences
            if (fencesBeginRegex.test(line)) {
              break;
            }

@kiyoka
Copy link
Contributor Author

kiyoka commented Jun 12, 2022

@UziTech @calculuschild
Thank you for your review.
It made it simpler.
I have committed to the fix.

Copy link
Contributor

@calculuschild calculuschild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is fine. I have ideas to maybe speed this up in the future but it will take a little more tinkering.

@UziTech UziTech changed the title Fix: Code after list without blank line (#1590) , Heading after list (#2406) fix: Code and heading after list without blank line Jun 13, 2022
@UziTech UziTech merged commit 15f3f15 into markedjs:master Jun 13, 2022
github-actions bot pushed a commit that referenced this pull request Jun 13, 2022
## [4.0.17](v4.0.16...v4.0.17) (2022-06-13)

### Bug Fixes

* Code and heading after list without blank line ([#2483](#2483)) ([15f3f15](15f3f15))
@github-actions
Copy link

🎉 This PR is included in version 4.0.17 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment