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

Complete revamp of markdown-gfm #7307

Merged
merged 32 commits into from
Aug 17, 2020
Merged

Complete revamp of markdown-gfm #7307

merged 32 commits into from
Aug 17, 2020

Conversation

fredck
Copy link
Contributor

@fredck fredck commented May 27, 2020

Suggested merge commit message (convention)

Feature (markdown-gfm): The markdown data processor has been totally revamped and updated to the most recent conversion libraries. Closes #5988.


Additional information

This PR is used in production within GitHub Writer.


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 maintained 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.


Original PR: ckeditor/ckeditor5-markdown-gfm#29

@jodator jodator self-requested a review August 14, 2020 11:25
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

Hi @fredck, thanks for contributing to the project!

Sorry, I couldn't resist 😉.

I had to do some code alignment to the what the team is used to so most of my changes are mostly cosmetics:

  1. We don't use foo && foo.bar && doSomethingWithFooBar( foo ) due to readability issues. Unfortunately our ESLint allows that: Re-enable no-unused-expressions ESLint rule #7843.
  2. I've moved RegExp and escape definitions out of TurndownService.prototype.escape definition - we can define them once, not per every TurndownService.prototype.escape call - improved memory management.
  3. I've refactored URL matching to use String.matchAll() - IMO it is a nicer API over the RegExp.exec + do...while. So just a preference.
  4. To ensure I don't broke anything I've added a test for escaping string between two URLs.

The PR is not ready to be merged yet since I need to fix API docs (I'll explain changes later) - but in short - you can't provide empty @module  jsdoc annotation - it must be a valid "path"-like entry, something like:

/** @module markdown-gfm/html2markdown/html2markdown */

@jodator
Copy link
Contributor

jodator commented Aug 14, 2020

ps.: The merge will happen soon, together with the rest of markdown-related PRs so it should be available in the upcoming release :todo:.

@jodator
Copy link
Contributor

jodator commented Aug 14, 2020

ps.: The merge will happen soon, together with the rest of markdown-related PRs so it should be available in the upcoming release 🎉.

@jodator
Copy link
Contributor

jodator commented Aug 17, 2020

The errors are on master as well :/ Merging as plugin has no own errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdown-GFM: the current implementation is outdated
2 participants