Move markdown-it parsing to /markbind

[ang-zeyu]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Documentation update
• [x] Other, please explain:

Resolves #969 - move attribute markdown parsing to /markbind, removing markdown-it as a dependency from vue-strap
Resolves #36 - standardise attribute naming for "header", "title", etc.

As a result,
Fixes #967 - some unexpected behaviours related to panel heading indexing
Combined with #966 ( closed ) - fix heading indexing for pure html headings, which is necessary for #969

Requires MarkBind/vue-strap#120 for the corresponding vue-strap template changes

What is the rationale for this request?

(1) To move all markdown parsing from the client to site building, which has benefits elaborated in #969 . Briefly,

• Better code cohesion & consistency ( where markdown parsing is done )
• Potentially faster client webpage loading ( due to not parsing markdown there )
• Smaller bundle size ( lack of markdown-it library from /vue-strap )

(2) Fix heading indexing for pure html headings ( originaly in #966 ). This is required for moving markdown parsing, as headings are now rendered in the dom as tags.

(3) Following the attribute parsing, attribute naming is conveniently standardised as well, and existing docs / srces / other usages updated. Deprecated attributes are still supported and documented. Components affected:

• dropdowns - "text" -> "header"
• boxes - "heading" -> "header"
• popovers - "title" -> "header"
• modals - "title" -> "header"

What changes did you make? (Give an overview)
(1) Introduce html heading id assigning identical to markdown-it-anchor for heading indexing, but for all types of headings. Consequently removed markdown-it-anchor plugin. Integrated this into _parse process.
(2) Introduced a generic per-component parser class housing all the new logic. Currently this only contains logic for parsing markdown in attributes. Integrated this into _parse process as well.

• It also houses all the attribute naming standardisation

Provide some example code that this change will affect:

The main changes are summarised in the _parse function on a high level.

  // this handles the heading indexing
  if ((/^h[1-6]$/).test(element.name) && !element.attribs.id) {
    const textContent = utils.getTextContent(element);
    const slugifiedHeading = slugify(textContent, { decamelize: false });

    ... <some omitted id conflict resolution code> ...
  ... <old switch case> ...

  // this handles the attribute parsing
  self.componentParser.parseVueComponents(element);

Is there anything you'd like reviewers to focus on?
ComponentParser is quite unit - testable. This pr is getting a little large though - should it be done in a follow up instead?

Testing instructions:
npm run test will verify the dom changes to the test files, which have been checked.

Proposed commit message: (wrap lines at 72 characters)
Move markdown-it parsing to /markbind

Markdown passed in attributes are being rendered in
the client.

This results in a dependency on the markdown parsing
library ( markdown-it ) on the client bloating the resulting
bundle size for /vue-strap.
It also slows webpage loading, especially on older devices.
Moreover, /markbind already parses some amount of
markdown in attributes for other purposes when building
the site. This results in a less cohesive codebase.

Let's move all markdown-it parsing to /markbind from
/vue-strap, passing the rendered html into vue slots.
Additionally, let's add heading indexing for pure html
headings, which is required for heading indexing to
function correctly after this change.

[ang-zeyu]

Hopefully these comments make reviewing easier!

[ang-zeyu]

Result of moving the vue-strap version of markdown-it over

[ang-zeyu]

This rule requires methods inside es6 classes to have some reference to this keyword,
which dosen't fit with current rules

[yamgent]

This rule exist, because these methods flagged by eslint do not need to be instance methods. They can instead be class methods instead, by putting a static keyword in front (e.g. static someClassMethods() {.

This rule can be quite useful in some situation. Possible to keep it enabled and use the static keyword where appropriate instead?

[ang-zeyu]

Needed for markdown-it-dimmed to work which is used in vue-strap version of markdown-it

[ang-zeyu]

Mostly attribute name standardisation for docs, with some misc fixes

[ang-zeyu]

I noticed this wasn't there, but was shown in the examples in the popover section, so I added it in

[ang-zeyu]

A side benefit of per file conflict resolution and moving header attribute parsing to dom

[ang-zeyu]

Ordering changed due to different id assigning location, but end result is as expected.
Last number 24 -> 26 due to two addition heading ids now assigned to the below panel's headers correctly

[ang-zeyu]

Some attribute names stayed in full the "header" vs "h" as using <slot "header">s was already a feature of the component

[ang-zeyu]

src/template/default/index.md had a pure html <h1> heading not indexed before

[ang-zeyu]

src/template/default/index.md had a pure html <h1> heading not indexed before

[yamgent]

This heading should not be indexed at all. Add a "no-index" class to it.

[yamgent]

Thanks for working on this. Will look into this soon.

In the meantime, since we now have a proper implementation, would you mind running these with the cs2103 website again? So that now we have an actual statistics of how much overhead is reduced + the increase in file sizes as a result of the shift.

[ang-zeyu]

The average ( roughly just from eyeballing ) increase seems to be much less than the initial estimated +20kb per file. The largest html file ( print.html ) increased by 21kb only.

The worst increase of these is forumParticipation.html, which has about 500 panels from a ctrl-f ( many of which don't have headings though ). Even then its a ~50kb increase

Diff of ls -s file sizes of .html files
sizeDiff.txt
https://pastebin.com/CUdN14dD

For/vue-strap, the file size change is as was before ( -= ~180kb ish ), which is expected since not much template changes were required

[damithc]

Thanks for tackling this as MarkBind is in need of some hardcore code/design improvements.

Is there any performance impact, w.r.t markbind build and live preview?

(3) Following the attribute parsing, attribute naming is conveniently standardised as well, and existing docs / srces / other usages updated. Deprecated attributes are still supported and documented. Components affected:

• dropdowns - "text" -> "header"
• boxes - "heading" -> "header"
• popovers - "title" -> "header"
• modals - "title" -> "header"

Note that we use header in another place. Should we use a different term here to avoid confusion?

[ang-zeyu]

Is there any performance impact, w.r.t markbind build and live preview?

Definitely, they seem pretty small though:

Before:
Build times: 156.406, 156.458, 156.58 = 156.48
Live reloads (text.md of 'software engineering' topic, which causes 9 files to rebuild): 11.512, 11.599, 11.488, 11.688, 11.557 = 11.569

After:
Builds times: 161.38 157.811 160.143 = 159.78 (+2%)
Live reloads (same file as before): 11.784, 11.418, 11.176, 11.66, 11.284 = 11.46 (likely due to background processes before)

Note that we use header in another place. Should we use a different term here to avoid confusion?

Thanks! didn't take note of this earlier.

Alternatives:

• title - short and semantic, but this isn't quite in line with whatwg spec.
• heading - may feel long, but it avoids the confusion
• name - a little ambiguous, since a 'name' can be naming other things in the component
• ❔

[acjh]

Note that we use header in another place. Should we use a different term here to avoid confusion?

Bootstrap 4's card has both header and title depending on how it's rendered: https://getbootstrap.com/docs/4.4/components/card/#header-and-footer

[damithc]

I assume the performance loss (although small) is worth the code quality gain?

w.r.t. header/title etc. I'm OK with any, even header if there is no concern about confusing with <header> tag elsewhere. You guys decide.

[ang-zeyu]

I assume the performance loss (although small) is worth the code quality gain?

Yup, in practice it should be hardly noticeable, if not negated by decreasing client side rendering and bundle size.

On performance, I think a good next step would be to get a more specific stats on how much each stage of page generation takes, that maybe runs only in a "dev" mode.

e.g. Individual statistics for includeFile, preRender, renderFile, postRender so we can focus on optimizing individuals parts.

Preferably, this comes after any architecture reorganizing though.

w.r.t. header/title etc. I'm OK with any, even header if there is no concern about confusing with <header> tag elsewhere. You guys decide.

Bootstrap 4's card has both header and title depending on how it's rendered

Hmm, I personally think title would be the best choice if not for its standard use in html. We don't necessarily have to comply with the spec though, since MarkBind isn't exactly standard html.

Otherwise, I think it'd be good to stick with header which is in line with what card-header (amongst its other xx-header classes) does in bootstrap (inserting it into some other distinct section at the top) as pointed out by @acjh .
Still, to avoid confusion maybe we could instead change the <header> tag to <heading> or <banner> next.

[acjh]

I recommend aligning with Bootstrap 4.

Each component can have its own header; a page header is distinct enough from a popover header.
I guess that's why Bootstrap uses card-header.

Please avoid heading, which is what <h1>-<h6> are; it really isn't interchangeable with header.
A short reading: http://altformat.org/index.asp?id=5&pid=389&ipname=GB

I would consider banner a component on its own.

[ang-zeyu]

I recommend aligning with Bootstrap 4.

Each component can have its own header; a page header is distinct enough from a popover header.
I guess that's why Bootstrap uses card-header.

Please avoid heading, which is what <h1>-<h6> are; it really isn't interchangeable with header.
A short reading: http://altformat.org/index.asp?id=5&pid=389&ipname=GB

Good read, thanks! Will go with header then.

Update commit is for correcting my misuse of _.hasIn in ComponentParser to _.has.

Will squash if its fine

[yamgent]

This PR is quite big. 😅 Is it possible for you to split the commits such that tests and docs update are in separate commits? The best commit organization would be that each commit contains the smallest possible change that cannot be broken down further.

Also a side remark: if there are any changes that are unrelated to the markdown-it parsing changes at all you might want to put them in separate PRs so that they can be merged quicker.

[ang-zeyu]

This PR is quite big. 😅 Is it possible for you to split the commits such that tests and docs update are in separate commits?

👍

The unit tests haven't yet been pr-ed though 😅, I could lump it in this pr as a separate commit, or as an entirely new pr.

Proposed commit structure (5 commits):

• Functional changes ( split into )
  • fix html heading indexing
  • move markdown-it parser changes + attribute standardisation
• npm run updatetest changes
• Unit test additions
• Docs changes

Also a side remark: if there are any changes that are unrelated to the markdown-it parsing changes at all you might want to put them in separate PRs so that they can be merged quicker.

I originally intended to continue with #966 ( heading indexing ), but found it rather coupled to the markdown parsing changes.

• Specifically, the id conflict resolution may result in panel anchors breaking temporarily till this PR if there are multiple headings with the same name.
• ( Because /vue-strap assigns new ids to panels at runtime when parsing the header attribute, which dosen't have said conflict resolution )

I could continue with #966 if breaking it temporarily is fine and makes it easier to review though; its also a minor issue that existed before this.
Otherwise I can also split it according to the above commit structure which is also a good compromise.

[yamgent]

• Functional changes ( split into )

  • fix html heading indexing
  • move markdown-it parser changes + attribute standardisation

• npm run updatetest changes

• Unit test additions

• Docs changes

Seems fine as a start. For implementation wise could break it down even further like:

• Importing over some parts of markdown-it code from vue-strap
• Implementing ComponentParser

Hopefully this way the commits themselves get easier to review on their own.

[ang-zeyu]

• Importing over some parts of markdown-it code from vue-strap
• Implementing ComponentParser

Hopefully this way the commits themselves get easier to review on their own.

Thanks for the suggestions!

Will include those in and see if it can't be split up further

[ang-zeyu]

Split and updated;
I seperated the html indexing doc updates and input vs expected test files in addition

[yamgent]

Thanks for breaking up the commits, some preliminary comments:

[yamgent]

We should encourage the use of ### by default, hence the change to <h3> and <h5> doesn't seem to be useful.

If we want to avoid getting it indexed, we can use the {.no-index} attribute for the output instead. I.e. the change should just be:

##### Feature Y <span class="badge badge-pill badge-success">stable</span> {.no-index}`

I understand that some other parts of the documents may be using <h1> and etc. This is due to legacy reasons (the documentation was ported from a source that had no access to markdown syntax). The standardization should be done in a separate PR if needed (in fact, I encourage opening an issue on the issue tracker since this can be tackled at any time).

[yamgent]

Seems like these changes can just be integrated into src/lib/markbind/src/lib/markdown-it/index.js? I didn't really understand why we have a separate version here...

[ang-zeyu]

I opted for maintaining parity with past behaviours for now for this, as the reasoning for the omission of some of the plugins are clear ( e.g. markdown-it-task-lists wouldn't be used in attributes ), while I'm not so sure about some other plugins.

Markdown-it plugins vue-strap has but MarkBind dosen't:

• markdown-it-sub
• markdown-it-sup

Markdown-it plugins MarkBind has but vue-strap dosen't:

• markdown-it-attrs
• markdown-it-footnotes
• markdown-it-linkify-images

Markdown-it plugins MarkBind has but vue-strap dosen't, but would not be used in attributes anyway:

• markdown-it-table-of-contents
• markdown-it-task-lists
• markdown-it-block-embed
• markdown-it-radio-button

It might be good to reach a standardisation decision for this in a separate PR instead, in case it has some side effects

[yamgent]

Ok good point, thanks for clarifying. You might want to put this as part of your commit message for this commit, so that it is clearer to future developers why there was a decision to make it separate, and that this is a temporary measure until we can finally merge both together (the most ideal case).

Also with that reasoning you had, markdown-it-attributes would actually not be a very useful name (it can be misread as a library that handles markdown attributes similar to markdown-it-attrs, rather than handling Vue attributes). Maybe it should be called vue-attribute-renderer or something else?

[yamgent]

This rule exist, because these methods flagged by eslint do not need to be instance methods. They can instead be class methods instead, by putting a static keyword in front (e.g. static someClassMethods() {.

This rule can be quite useful in some situation. Possible to keep it enabled and use the static keyword where appropriate instead?

[yamgent]

I have combed through the commit Add unit tests for ComponentParser, the specification seems pretty satisfactory, I will do another look at the implementation of ComponentParser tomorrow.

[yamgent]

Ok good point, thanks for clarifying. You might want to put this as part of your commit message for this commit, so that it is clearer to future developers why there was a decision to make it separate, and that this is a temporary measure until we can finally merge both together (the most ideal case).

Also with that reasoning you had, markdown-it-attributes would actually not be a very useful name (it can be misread as a library that handles markdown attributes similar to markdown-it-attrs, rather than handling Vue attributes). Maybe it should be called vue-attribute-renderer or something else?

[ang-zeyu]

Updated:

• markdown-it-attributes naming to vue-attribute-renderer, and the corresponding commit message
• Removed ComponentParser class in favour of simple module function exports, as the only instance variable being used was the error handler. Hence it is now more of a supporting collection of utility methods to parser.js.
• Reverted change to 'class-methods-use-this' eslint rule
• Reverted <h4/5> in badges.mbdf to use markdown headings
• Added missing comment base case check for getTextContent ( found when checking cheerio source code ), thanks @yamgent!

Um I can't find the original source code in cheerio, can you point me to the source?

Code identical to the cheerio version we are currently using:
https://github.com/cheeriojs/cheerio/blob/765cdaaac56acdaf8779867c6205b7edde25e91f/lib/static.js#L107

Updated code after cheerio also excluded <script> and <style> tags from the concatenation ( not the version we are using though ):
https://github.com/cheeriojs/cheerio/blob/4afae9af47f2a55cd00aa6a9feddbb8c7b80ab8b/lib/static.js#L107

The code is quite different, as cheerio uses their internal root() function to recursively retrieve the next node in the dfs. This is not possible here, and not needed, as the loaded cheerio dom is not available in the _parse process.

I used a stack for the dfs here to make it easier to read ( personal opinion ), but I could change it to use recursive calls if that's preferred.

[ang-zeyu]

Rebased, only updated expected test files for new expressive layouts test site ( 2nd last commit )

[ang-zeyu]

Updated to resolve conflicts with es6 classes pr

[yamgent]

Thanks for your patience! Overall the implementation is pretty satisfactory. There could be some minor details hiccup that I missed, but in view of time, I am OK with getting this merged first, and then doing touch up if necessary in the future.

Some final minor nits:

[yamgent]

The text should be shorten (we can also shorten the actual displayed text). The code snippet has a scrollbar now.

[yamgent]

This heading should not be indexed at all. Add a "no-index" class to it.

[yamgent]

.slice() will do.

[ang-zeyu]

Some final minor nits:

Updated.

Thanks for your patience! Overall the implementation is pretty satisfactory. There could be some minor details hiccup that I missed,

Thanks for taking your time to review this as well! Its quite a large PR.

but in view of time, I am OK with getting this merged first, and then doing touch up if necessary in the future.

I'm not in a hurry to get this merged though, I'm fine with touching it up later as well if needed, but if you feel it still needs more reviewing on your end, please do haha 😁