Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Complete revamp of the code #29

Closed
wants to merge 24 commits into from
Closed

Complete revamp of the code #29

wants to merge 24 commits into from

Conversation

fredck
Copy link

@fredck fredck commented Dec 16, 2019

Feature: The markdown data processor has been totally revamped and updated to the most recent conversion libraries. Closes ckeditor/ckeditor5#5988.


Additional information

The main updates related to this PR are the use of the most up-to-date libraries for markdown conversion:

  • The marked library was 5 years old. Meanwhile it became a solid, well maintanied and active project.
  • The to-markdown library dies and reborn, now named Turndown. It is a very active and beautiful project, maintained by many and reaching a great level of quality.

I've also removed the libraries from inside our code, making them normal dependencies.

One important impact of the updated on the libraries is that many tests started to fail. That's because the libraries are much more standard oriented now and less interested on the markdown-madness of accepting whatever is given.

All in all, the quality of the implementation is definitely higher.

@fredck
Copy link
Author

fredck commented Dec 18, 2019

Added a new commit that fixes ckeditor/ckeditor5#5997.

@fredck
Copy link
Author

fredck commented Dec 18, 2019

Coverage is now at 100%.

@fredck
Copy link
Author

fredck commented Feb 25, 2020

Force-pushed a rebase over v17.0.0.

@fredck
Copy link
Author

fredck commented Feb 25, 2020

Force-pushed a rebase onto master to fix conflicts recently introduced in tests.

@coveralls
Copy link

coveralls commented Feb 25, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 61b514a on t/ckeditor5/5988 into a05faa6 on master.

@fredck
Copy link
Author

fredck commented May 2, 2020

Force-pushed a rebase over v19.0.0.

@spirobel
Copy link

First of all: Thank you very much for doing this! I am working on adding ckeditor to the discourse.org forum software. https://github.com/spirobel/discourse-basic-editor
I am not sure if this is the right place to bring this up, or if I should open a separate issue. It would be very nice, if we could add marked and turndown rules via the config options to the editor.create function. I already created a small plugin that lets me add new toolbar buttons this way from my discourse plugin: https://github.com/spirobel/ckeditor5-build-classic/blob/stable/src/ckeditor.js I can also create a PR if you want. :D

@fredck
Copy link
Author

fredck commented May 18, 2020

It would be very nice, if we could add marked and turndown rules via the config options to the editor.create function.

@spirobel, thanks for sharing.

The goal with this refactoring is creating a solution that works with markdown that is supported by GFM and GitHub. It is in use by GitHub Writer.

What's your intention by being able to add custom rules to marked and turndown? I mean, I think it is an idea to consider, to expose them for customization, but I would love to hear real world use cases for it.

Finally, the first step here is having this PR accepted by @ckeditor/ckeditor-5-core-developers. Then, you may think about opening an issue for further development of this plugin. You may also let them know that you want to see this PR merged by commenting on ckeditor/ckeditor5#5988. Thanks!

@spirobel
Copy link

spirobel commented May 18, 2020

The real world use case is that discourse also supports bbcode and bbcode like structures for example you can quote someone like this [quote="username, post:1, topic:32"] test message that is quoted [/quote] and his username will show up in the quote. To support this kind of thing "to and from markdown" rules need to be created. Do you think it would be feasible to implement it this way?
The thing I forgot to mention: When writing discourse plugins it is not easy to add npm modules to the project, because of this custom asset pipeline. But still most of the code should be in the repo of the discourse plugin. Thats why I am building ckeditor in another repo and copy the build/ckeditor.js into my discourse plugin repo. In this function I set up ckeditor and push in my toolbar buttons "from the outside" https://github.com/spirobel/discourse-basic-editor/blob/72166b1af099e272e53d36eaeb66c01a917efcd2/assets/javascripts/discourse/components/b-editor.js.es6#L34 In a similar fashion I whould like to go about the "bbcode" structures. I hope you can understand what I want to say 😄

@poorgeek
Copy link

Piling on to the case for supporting custom rules. We use several proposed extensions to Markdown in our documents which is already supported by some renderers; e.g. MarkDig. Until now this has been a one-way rendering only, but I've just started looking at ckeditor for the same reasons as @spirobel and want to extend the markdown to support some of the additional proposed syntax out there. Supporting custom rules within the plugin would certainly save me a lot of time.

@spirobel
Copy link

spirobel commented May 18, 2020

I have been thinking about this a bit more. Discourse has a markdown pipeline in the backend, that cooks raw markdown posts to html. The posts get baked (thats how they call it) once on submit and get rebaked under some circumstances. They also support html baking for emails. so it might be possible to add a "html baking pipeline" next to the markdown. I agree with the post @fredck wrote about html as a RTE format:https://medium.com/content-uneditable/a-standard-for-rich-text-data-4b3a507af552 I think in the long run its too much of a hassle to have all the custom extensions to their markdown and convert to and from it. It will get harder and harder to add features. The issue is that there will be legacy posts in markdown, that should still be editable. So upstream rules for these extensions would be necessary. But also it would be nice to output with the htmldataprocessor. So for some posts the htmldataprocessor can be used and for some the import will need to happen from markdown and the output will be in html. I dont know how it will turn out in the end. I will use my discourse plugin on my website and continue to work on it. I will also try to find some other users in the discourse community and ask for their opinion on how to proceed and which features they need. In the short term importing and exporting to markdown will be fine. It works surprisingly well out of the box and its already a big usability gain. Another way to get around this would be to use the cooked html posts as input and work from there. But I dont know yet if thats feasible. This way there would be no need for these custom rules in my case.

@fredck
Copy link
Author

fredck commented May 19, 2020

@spirobel, @poorgeek, thanks for the details.

Both of you seem to point to the same direction: the need of a data processor which is specific to a system (non-generic).

Although the ckeditor5-markdown-gfm DP sounds like generic, its goal is supporting a very-specific system (implementation, if you wish) - the GitHub Flavored Markdown. It is not intended to be a generic markdown implementation. In fact, for example, we implement some quirks specific to GitHub.

I'm not saying that enlarging it so it can be further customized (or hacked) doesn't make sense. But, would this be the right approach? Wouldn't it be "more correct" if instead you guys would fork the GFM Markdown data processor and create, based on it, a perfectly crafted "Discourse Markdown" or "My System Markdown" data processor?

@spirobel
Copy link

I think in theory it makes sense, but in practice a fork can mean a lot of work. As this PR shows, libraries need to be updated and maintained. I think the quirks and customizations will be addressed in arrays of to/from-markdown conversion rules. It would be nice to be able to create these rules without the need to maintain a custom fork. I will investigate a solution in the following order: 1.try to use html as a format 2. if 1.does not work create a fork of this processor. Maybe if thats the case we can think about this more. I think when it comes to Markdown its almost the same everywhere. Its just the "bbcode like" features that got build on top. Maybe there is a common core, that can be extracted out and all the different "My System Markdowns 😄 " can build on. I just hope using html as a format can work. The variety of "markdown addons" shows it is just not generic enough to work well as an rte format.

@oliverguenther
Copy link

oliverguenther commented May 19, 2020

Just chiming in here on this interesting discussion. We do use a fork from gfm markdown at OpenProject that uses turndown, we definitely recommend using that library since we use it in production with CKEditor5 for over two years now.

Most of the fixes for various quirks are indeed performed with custom turndown rules (keeping the original HTML table for example even though GFM supports tables - but their spec I found to be too brittle and limited to work).

We still need additional pre and postprocessing hooks for some of the quirks from both CKEditor (or some of our custom plugins such as a code block with CodeMirror), that would probbaly not work even when using this upstream version. I doubt we could make much use of an official GFM DP with the turndown rules API exposed, we would need to have hooks before and after the DP is called. This might of course work if provided by CKEditor independently of GFM, but for now I'm quite happy with our custom fork.

@fredck
Copy link
Author

fredck commented May 27, 2020

The PR has been moved to ckeditor/ckeditor5#7307, following the CKEditor 5 move to monorepo.

For those interested, be sure to add 👍 and further feedback there.

@fredck fredck closed this May 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdown-GFM: the current implementation is outdated
5 participants