Replace marked with commonmark #575

kyrias · 2016-11-29T22:14:34Z

Marked has some annoying bugs, and the author is inactive, so replace it
with commonmark.js, which is the reference JavaScript implementation of
CommonMark. CommonMark is also preferable since it has a specification,
and a conformance test suite to make sure that parsers are correct.

Marked has some annoying bugs, and the author is inactive, so replace it with commonmark.js, which is the reference JavaScript implementation of CommonMark. CommonMark is also preferable since it has a specification, and a conformance test suite to make sure that parsers are correct. Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>

matrixbot · 2016-11-29T22:14:36Z

Can one of the admins verify this patch?

dbkr · 2016-11-29T22:55:32Z

src/Markdown.js

+                par = par.parent
+            }
+            if (par.firstChild != par.lastChild) {
+                real_paragraph.bind(this)(node, entering);


Did you want real_paragraph.call(this, node, entering) here? (It's effectively the same, just doesn't make a bound function).

Cool, don't do a lot of JS.

dbkr · 2016-11-29T23:08:10Z

This generally seems like a pretty good idea, although it will break a couple of little bits of syntax we currently support from GFM. To be precise, I believe these are strikethrough and tables.

If we like, we can turn off the 'safe' option to allow HTML tags through, which will just let people type HTML into the box and it end up in the message, which is no worse than you could do by posting the same HTML using the API directly.

OTOH, typing mixed HR characters like -_- will no longer make HRs!

Anyone object?

ara4n · 2016-11-29T23:10:54Z

hum. we'd have to do it across the board, if at all...

…

On 29 Nov 2016, at 23:08, David Baker ***@***.***> wrote: Assigned #575 to @ara4n. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kyrias · 2016-11-30T00:02:30Z

Yeah, I'm planning on looking into it for the rest too, but wanted to see if this would be accepted before doing the extra work.

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>

kegsay · 2016-11-30T11:22:55Z

Personally, I'd rather keep using GFM - it's more feature-rich than Commonmark.

Marked has some annoying bugs

Which in particular are affecting you?

CommonMark is also preferable since it has a specification, and a conformance test suite to make sure that parsers are correct.

Whilst that is nice, I don't agree that we should ~~cripple~~remove features on the basis that there's a simpler but more reliable library. I would certainly miss being able to strike-through to articulate my point :)

richvdh · 2016-11-30T11:28:44Z

For me, the consistent spec and better-maintained library offset the loss of strikethrough.

kyrias · 2016-11-30T13:42:30Z

Two things off the top of my head being three or more, with any combination of *, -, and _ causing a hr, and escaping underscores inside words being broken.

And with HTML enabled, which as mentioned won't allow anything malicious that wouldn't otherwise be possible, you can use <s>foo</s> (which even GitHub renders).

kyrias · 2016-12-02T20:31:31Z

I fixed another markdown bug related to escaping markdown characters btw, but it either depends on this patch, or needs to be reimplemented for the current code, which I would rather not work on unless this is rejected.

kegsay · 2016-12-05T15:35:39Z

Looks like I'm the only one holding out for GFM. Use of <s>stuff</s> is an acceptable replacement for ~~~ imo, and the consistent implementations would be nice. I'm not convinced that swapping to Commonmark will fix all compatibility issues, as parsers f.e language are in various states of development, but at least they're all aimed at the same target this way.

I'm happy for us to swap to Commonmark.

richvdh · 2016-12-05T17:02:34Z

Matthew says:

we need input from Giom & Yannick
for instance, istr that Yannick is firing up an entire v8 VM in order to run marked on Android atm(!)
in order to get consistent syntax with the web
as the Android GFM libraries had lots of different edge cases
so he may either be very happy or very betrayed if we now go and switch to commonmark

@ylecollen, @giomfo: wdyt?

giomfo · 2016-12-12T10:31:16Z

Manu and I agree to swap to Commonmark in Riot iOS (A new issue has been filed: element-hq/element-ios#862)

ylecollen · 2016-12-12T10:37:05Z

The mobile clients have a markdown parser and a markdown renderer.

This update will only update the parser side.

manuroe · 2016-12-12T10:51:58Z

I would be happy to use the same library on all platforms as it was a pain to make iOS and Android MD work like the web client.

richvdh · 2016-12-12T11:01:18Z

Ok, it sounds like we are all agreed in principle that the switch to CommonMark is a good idea \o/. Now all we have to do is make it happen.

I think consistency is important so I think we shouldn't merge this patch until we have support in the iOS and Android apps ready to go too.

@kyrias: did you have any interest in replicating this on the mobile apps?

kyrias · 2016-12-12T14:38:20Z

I have never done any Android/iOS development, but I could certainly try if no one else gets to it before me.

ylecollen · 2016-12-12T15:20:54Z

Is there any unit tests set to ensure that the client parses properly ?

kyrias · 2016-12-13T10:05:41Z

https://github.com/jgm/CommonMark is the commonmark test suite, but there's none for matrix-react-sdk.

kyrias · 2017-01-03T20:42:34Z

I haven't really had the time or energy to even start looking at the Android or iOS part of this, but it seems like there's a pure Java implementation that works on Android, and either a pure Swift implementation or the C reference implementation for iOS.

kegsay · 2017-01-17T11:56:50Z

As per element-hq/element-web#2870 (comment) - We're pushing ahead with CommonMark. Merging.

manuroe · 2017-02-27T16:12:34Z

FTR, it has been added to Matrix iOS kit thanks to matrix-org/matrix-ios-kit#248

dbkr reviewed Nov 29, 2016

View reviewed changes

dbkr assigned ara4n and richvdh Nov 29, 2016

Markdown: Use .call instead of .bind

5f160d2

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>

richvdh assigned giomfo and ylecollen Dec 6, 2016

giomfo mentioned this pull request Dec 12, 2016

Replace GHMarkdownParser with Commonmark element-hq/element-ios#862

Closed

ylecollen mentioned this pull request Dec 12, 2016

Replace GHMarkdownParser with Commonmark element-hq/riot-android#779

Open

richvdh unassigned giomfo and ylecollen Dec 12, 2016

richvdh removed their assignment Dec 21, 2016

kyrias mentioned this pull request Jan 3, 2017

Riot fails to remove backslashes if the resulting message is entirely plain text element-hq/element-web#2870

Closed

kegsay merged commit fcb1d7a into matrix-org:develop Jan 17, 2017

kyrias deleted the commonmark branch January 17, 2017 20:04

kegsay mentioned this pull request Jan 18, 2017

Fix escaping markdown by rendering plaintext #622

Merged

dbkr mentioned this pull request Jan 4, 2018

RTE inserts spurious backslashes into URLs (including room aliases, etc) in code blocks element-hq/element-web#4809

Closed

jryans mentioned this pull request May 7, 2020

Should we go back to GFM rather than CommonMark? (The general Markdown bug) element-hq/element-meta#330

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace marked with commonmark #575

Replace marked with commonmark #575

kyrias commented Nov 29, 2016

matrixbot commented Nov 29, 2016

dbkr Nov 29, 2016

kyrias Nov 30, 2016

dbkr commented Nov 29, 2016

ara4n commented Nov 29, 2016 via email

kyrias commented Nov 30, 2016

kegsay commented Nov 30, 2016

richvdh commented Nov 30, 2016

kyrias commented Nov 30, 2016

kyrias commented Dec 2, 2016

kegsay commented Dec 5, 2016

richvdh commented Dec 5, 2016

giomfo commented Dec 12, 2016

ylecollen commented Dec 12, 2016

manuroe commented Dec 12, 2016 •

edited

Loading

richvdh commented Dec 12, 2016

kyrias commented Dec 12, 2016

ylecollen commented Dec 12, 2016

kyrias commented Dec 13, 2016

kyrias commented Jan 3, 2017

kegsay commented Jan 17, 2017

manuroe commented Feb 27, 2017

Replace marked with commonmark #575

Replace marked with commonmark #575

Conversation

kyrias commented Nov 29, 2016

matrixbot commented Nov 29, 2016

dbkr Nov 29, 2016

Choose a reason for hiding this comment

kyrias Nov 30, 2016

Choose a reason for hiding this comment

dbkr commented Nov 29, 2016

ara4n commented Nov 29, 2016 via email

kyrias commented Nov 30, 2016

kegsay commented Nov 30, 2016

richvdh commented Nov 30, 2016

kyrias commented Nov 30, 2016

kyrias commented Dec 2, 2016

kegsay commented Dec 5, 2016

richvdh commented Dec 5, 2016

giomfo commented Dec 12, 2016

ylecollen commented Dec 12, 2016

manuroe commented Dec 12, 2016 • edited Loading

richvdh commented Dec 12, 2016

kyrias commented Dec 12, 2016

ylecollen commented Dec 12, 2016

kyrias commented Dec 13, 2016

kyrias commented Jan 3, 2017

kegsay commented Jan 17, 2017

manuroe commented Feb 27, 2017

manuroe commented Dec 12, 2016 •

edited

Loading