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

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

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 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 commented Dec 15, 2021

does this fix #2278 as well?

@imchell
Copy link
Contributor Author

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 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 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 commented Dec 17, 2021

Thanks. It's a better solution.

@UziTech
Copy link
Member

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 commented Dec 18, 2021

@UziTech updated 👀

Copy link
Member

@UziTech UziTech left a comment

Choose a reason for hiding this comment

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

Thanks! 💯

@UziTech UziTech merged commit f82ea2c into markedjs:master Dec 19, 2021
github-actions bot pushed a commit that referenced this pull request 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

🎉 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
Development

Successfully merging this pull request may close these issues.

Error thrown if there's a space on a newline after a table
3 participants