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: spaces on a newline after a table #2319

Merged
merged 10 commits into from Dec 19, 2021
Merged

fix: spaces on a newline after a table #2319

merged 10 commits into from Dec 19, 2021

Conversation

@imchell
Copy link
Contributor

@imchell imchell commented Dec 11, 2021

Marked version: 4.0.7

Markdown flavor: Markdown.pl|CommonMark|GitHub Flavored Markdown|n/a

Description

  • Fixes #2249
    The problem can be solved by removing the last table row with no data. The row matching /^ {1,4}$/ should be removed. There is no need to deal with /^ {5,}$/ since more than 4 spaces would be parsed to a fenced code block.

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for 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.

@vercel
Copy link

@vercel vercel bot commented Dec 11, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/markedjs/markedjs/8R4yxox13gcfwaQktGJmJxcEEPoW
Preview: https://markedjs-git-fork-imchell-master-markedjs.vercel.app

@UziTech
Copy link
Member

@UziTech UziTech commented Dec 15, 2021

does this fix #2278 as well?

@imchell
Copy link
Contributor Author

@imchell imchell commented Dec 16, 2021

No, it is for #2249 solely. #2278 is getting TypeError exception for a similar reason, but the parsing result is not the same as commonmark.js even the exception is fixed. I will look into that.

@imchell
Copy link
Contributor Author

@imchell imchell commented Dec 17, 2021

There are no exceptions both for #2249 and #2278 now, but the parsing result of #2278 may differ from commonmark.js because of the ambiguity between gfm table and setext heading ( #1499 ).

@UziTech
Copy link
Member

@UziTech UziTech commented Dec 17, 2021

after a little bit of experimenting it looks like a better fix would be to change the line:

-        rows: cap[3] ? cap[3].replace(/\n$/, '').split('\n') : []
+        rows: cap[3] ? cap[3].replace(/\n[ \t]*$/, '').split('\n') : []

basically removing any trailing space characters before creating rows.

This would allow removing more than 4 spaces without affecting fenced code blocks inside the table.

@imchell
Copy link
Contributor Author

@imchell imchell commented Dec 17, 2021

Thanks. It's a better solution.

@UziTech
Copy link
Member

@UziTech UziTech commented Dec 18, 2021

@imchell can you make that change and leave the test to make sure it works?

@imchell
Copy link
Contributor Author

@imchell imchell commented Dec 18, 2021

@UziTech updated 👀

Copy link
Member

@UziTech UziTech left a comment

Thanks! 💯

@UziTech UziTech merged commit f82ea2c into markedjs:master Dec 19, 2021
10 checks passed
github-actions bot pushed a commit that referenced this issue Dec 19, 2021
## [4.0.8](v4.0.7...v4.0.8) (2021-12-19)

### Bug Fixes

* spaces on a newline after a table ([#2319](#2319)) ([f82ea2c](f82ea2c))
@github-actions
Copy link

@github-actions github-actions bot commented Dec 19, 2021

🎉 This PR is included in version 4.0.8 🎉

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
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants