Upgraded to Marked v1.1.1

[fredck]

Suggested merge commit message (convention)

Other: Upgraded to Marked v1.1.1. Closes #7850.

[fredck]

This PR is incomplete.

For now, it just upgraded our code to use the new API introduced with Marked v1.0.0.

The upgrade per se is complete as the latest Marked is properly run. The problem is that some small changes on the behavior of Marked made some of our tests red and they need to be checked.

This makes me wonder if we should really have lots of tests to certify the code produced by third-party libraries. It's like introducing tests to features that we use in lodash, which we don't do.

Questions

Shouldn't we just take these libraries as good and trust that they'll do their job (just like we trust many others)?

Shouldn't we have tests just for the code that we do, like our configurations and customizations to such libraries?

What's the point of maintaining an incomplete (and maybe even biased) set of tests for the markdown <=> html conversion?

[jodator]

It is hard to tell. In most cases we just do that and if the behavior change we match it in the code. Here we have GFM so the intention is to have the output as close to GH as possible.

The first thing I've noticed that the broken tests behave this way on GH so we caught incompatibility, which is good. I'd see those tests here as we already have them. I'd feel way safer with upgrading the external data processor having those. I'm for keeping those tests do have described how we want GFM data processor to behave.

The failing tests are 2 cases:

1. Space at the beginning of a <code> is stripped on GH - weird to me - maybe a GH bug or just they wanted this to behave. But this is how GH works.
2. The space after a <code> inside <blockquote> - in this case, it looks like a bug - I don't know why this space is inserted there.

I don't recall how GHWriter catches the incompatible markup to issue a warning. I'd suspect that in those cases it might fail due to input !== output. So this might be a question for you @fredck - will that be OK?

If it would be just Markdown plugin then I'd go for 🤷‍♂️ probably. Here we advertise this plugin as GFM so I'm leaning towards achieving ">99.9%" compatibility. Or maybe going by a rule that ~80% use-cases should not trigger incompatibility warning in dependant apps.

For now, we could go with whatever man as this is an "experimental" plugin. If we'd go with changing those 2 tests, at least we should move them to another suite which gathers "GFM incompatibilities".

---

To sum up we could:

1. Don't upgrade to 1.x just now.
2. Upgrade to 1.x and redefine failing tests (accommodate the 1.x changes but have them clearly marked as incompatible with GH).
3. Upgrade to 1.x and fix the behavior somehow (+maybe fill a bug for marked or turndown).

//cc @Reinmar

[jodator]

@ckeditor/qa-team could you check how the Markdown editor behaves now:

1. On current master (we've integrated recent changes in markdown processing).
2. With this PR.

I've added a manual test for the markdown editor in 0bd756c.

[fredck]

I don't recall how GHWriter catches the incompatible markup to issue a warning. I'd suspect that in those cases it might fail due to input !== output. So this might be a question for you @fredck - will that be OK?

Yes, that's the way we do the check. The check is actually saying: "Hey, is this markdown produced by GHW? No, so let me see if there is any potential loss." Then, once the data is saved with GHW, the warning doesn't show up anymore.

In any case, the pointed out cases are just part of hundreds of other incompatibility cases (which we don't test). So, I would not be focused on them, specifically.

[fredck]

The failing tests are 2 cases:

1. Space at the beginning of a <code> is stripped on GH - weird to me - maybe a GH bug or just they wanted this to behave. But this is how GH works.

GH keeps the space intact. It is curious to see that Marked indeed fixed the behavior (so the upgrade is good and our tests are partially wrong) but in the other hand, it exposed an issue with Turndown (just paste the HTML produced with Marked here).

So, IMHO, if we care about this specific case for some reason, we should report it to Turndown and move on with the upgrade.

1. The space after a <code> inside <blockquote> - in this case, it looks like a bug - I don't know why this space is inserted there.

The curious thing is that I'm not able to have that space by using the Marked demo for this case.

Is there any chance that our test system or even CKE5 itself is appending that space?

[fredck]

3. Upgrade to 1.x and fix the behavior somehow (+maybe fill a bug for marked or turndown).

I think trying to fix every single case we meet at our side would be pointless. We really have hundreds of unknown cases, so the best we can do is opening issues on the third-party projects and maybe even contributing PRs to them to fix things that we're interested in. I've taken this approach when I faced "real world" issues in GHW.

[fredck]

2. Upgrade to 1.x and redefine failing tests (accommodate the 1.x changes but have them clearly marked as incompatible with GH).

I would go with always keeping these libraries up-to-date. I believe the benefits strongly overcome possible isolated issues.

[jodator]

To sum up:

1. For the most-used & know feature that uses this plugin (GithubWriter) there's no downside of having those issues fixed in tests only?
2. If so let's clear move them as in Upgraded to Marked v1.1.1 #7859 - so let's keep that knowledge if it behaves differently than GH. Otherwise just rewrite them.
3. I'm also for having up-to-date dependencies.

[jodator]

Is there any chance that our test system or even CKE5 itself is appending that space?

It is - reported here: #7863

[jodator]

@fredck I've identified one potential bug for blockqouote > pre handling (#7863). The <code> handling was redefined. I'll wait for @ckeditor/qa-team to give me green light here and will be merging it 🎉 .

[Mgsy]

We've tested it with @FilipTokarski and it seems to work fine on master and with changes introduced by this PR 🎉 .

However, I've noticed an interesting bug (but I believe it should be reported as a separated issue, as it occurs also on master). After adding Markdown plugin to our all features manual test, the initial content is treated as a code snippet.

With the Markdown plugin:

Without the Markdown plugin:

[jodator]

Thanks!

However, I've noticed an interesting bug (but I believe it should be reported as a separated issue, as it occurs also on master). After adding Markdown plugin to our all features manual test, the initial content is treated as a code snippet.

That's interesting, but for sure a separate issue as I don't think that severity of that is that big.