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

V3 baseline #1403

Merged
merged 17 commits into from
Dec 12, 2020
Merged

V3 baseline #1403

merged 17 commits into from
Dec 12, 2020

Conversation

ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented Dec 5, 2020

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

The architecture involves too much redundant processing; And the order of these processing stages is far too coupled.

This PR revolves around following this architecture: nunjucks -> markdown -> html, and also overhauls the layouts system in favour of one-file expressive layouts per #1308 at the same time (only because it makes the rewrite simpler =P)

Some plugin interface tweaks inline / required by the architecture simplification are also done

Resolves #1308
Resolves #1240
Resolves #539
Resolves #1188
Resolves #973
Resolves #810
Fixes #972

Related:
#896

Overview of changes:

  • oop-fied codebase (new manager classes: LayoutManager, ExternalManager, PluginManager new object classes Layout, External, Plugin)
  • simpler architecture that always follows: nunjucks -> markdown -> html
    • nodePreprocessor and nodeProcessor is combined into the html stage
  • plugin system tweaks
  • major performance improvements ~3x
  • updated layouts docs

Anything you'd like to highlight / discuss:

Todos:

  • all done

Testing instructions:
manually tested layouts system

Proposed commit message: (wrap lines at 72 characters)
Rebase - commits need to be merged as one but are quite distinct


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes 🤔 bigger than I'd like but its far too coupled to deliver separately
  • Pinged someone for a review!

@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented Dec 5, 2020

@damithc @acjh any thoughts on the layouts changes?

https://deploy-preview-1403--markbind-master.netlify.app/userguide/tweakingthepagestructure

So far I've noticed:

Its something we can reduce the work needed in template sites though, and may be warranted by the flexibility provided. (many interesting layouts will be made possible by this)

Not sure if there are more alternatives to alleviate this though

@ang-zeyu ang-zeyu added this to Nice to Haves in Big Picture via automation Dec 5, 2020
@ang-zeyu ang-zeyu moved this from Nice to Haves to In progress in Big Picture Dec 5, 2020
@damithc
Copy link
Contributor

damithc commented Dec 5, 2020

  • additional work for fixed headers is now needed - the fixed-header-padding class must be added by the user to the appropriate elements

In every page? Or just in the layout pages?

I noticed <span id="title" class="d-none">{{ title }}</span> being added to many pages. Is it quick of your own site or something authors have to do in every page in every site?

@@ -1,9 +1,10 @@
<variable name="title" id="title">Using Components</variable>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In every page? Or just in the layout pages?

Just layouts : )

Still something extra to take note of for the author though

I noticed <span id="title" class="d-none">{{ title }}</span> being added to many pages. Is it quick of your own site or something authors have to do in every page in every site?

This one would be belong to the page <variable> / <import> removal pr (to be split), there's no way to set the id for a nunjucks tag so had to create another <span>. The title id is used for the userguide macro that creates the left / right navigation buttons at the bottom

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it.

@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented Dec 7, 2020

Any thoughts on the docs / feature set? @damithc

44d8167

I'll give the code a few more looks here and there this week, then maybe merge it in this weekend otherwise.

@damithc
Copy link
Contributor

damithc commented Dec 7, 2020

Any thoughts on the docs / feature set? @damithc

Looks fine for me 👍
Any idea how much change this will require in CS2103 website?

@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented Dec 7, 2020

Looks fine for me 👍
Any idea how much change this will require in CS2103 website?

Hmm from what I can see there aren't a lot of layouts, so most of the work should be from converting page <variable> / <import>s to nunjucks {% set / import %}. Would depend on how many of those you have =X

For layouts you can re-run markbind init and copy over the default layout as a baseline, then 'simply <include>' them, and ctrl-R the layout names

Also the old attribute names which have been deprecated for a while now (title, text, ...) may be removed in a later PR (or we can postpone maybe, I don't think its affecting maintenance too much)

Layouts require the author to maintain numerous separate files (header,
footer, ...) which are combined in an opaque, restrictive manner to
form the skeleton for a page.

This increases the burden of author maintenance, with multiple files
and directories to keep track of.
It also restricts the author from forming layouts that require more
expressive structures that cannot be formed with the few fixed files.

Let's move to a simpler, one-file layout system which allows the author
to position the page content anywhere within the layout with a nunjucks
variable.
This also lifts the restrictions imposed by the way layout files are
previously combined, allowing more expressive page skeletons to be
formed.
Site navigation menus can only be formed in the navigation layout
file.

With the shift to a one-file layout system, let's extract site
navigation menu logic into a component.
This allows the author to place the menu anywhere within the layout
file.
MarkBind's html processing consist two distinct stages - include and
render.

In the first, <include>s are preprocessed, then the page is rendered
with markdown-it.
This necessitates markdown-htmlparser patches, which are difficult to
write.
This also poses issues in incorporating more expressive markdown-it
plugins which heavily conflict with html parsing.

Let's combine these two html processing stages to remedy this.

The result also, is a simpler architecture surrounding a recursive
process that always follows nunjucks -> markdown -> html processing.
With the html include and render stages merged, frontMatter collection
must be incorporated into the singular html processing stage, for
frontMatter from <include>s to be collected correctly.

This is also enabled by the new layouts system implementation, which
processes layouts independently from pages. Thus,  we may resolve the
layout of the page even after processing the page's content.
To reduce the repeat boilerplate code in layouts and pages, let's
extract temporary style insertion for FOUC into the html processing
stage.
Source files are read directly by MarkBind before passing to nunjucks
for processing.
This incurs an extra unnecessary step.

Nunjucks compiles a template into a javascript function, then uses it
to render the template.
This compilation process is costly, which is alleviated by nunjucks'
internal cache.
The nunjucks.renderString call is however unable to take advantage of
this cache.

Let's utilize the nunjucks.render call instead, thereby removing the
extra step, and allowing the cache to be used.
Files referenced by the 'src' attribute in panels are generated and
output as separate files, and dynamically retrieved at runtime in the
browser.

Such generation processes can be extracted into an 'External'
abstraction, facilitating future similar processes for other components
besides panels.

Let's create the External, and ExternalManager classes to consolidate
such generation logic.
With the merger of the include and render html stages, many breaking
changes are implied for the existing plugin interfaces.

For example, the preRender plugin hook is no longer applicable, and
postRender is no longer executed on the entire page content that is
surrounded by layout content.
Moreover, the frontMatter parameter is made inaccessible to many
plugin hooks.

Let's tweak the plugin system as such for the modified architecture.

The frontMatter parameter is removed from many plugin hooks.

In place of the plugin getSources hook - which also no longer has
access to the frontMatter, and getSpecialTags hook, let's introduce the
tagConfig plugin interface. This interfaces serves the same purpose and
consolidates the two hooks, allowing plugin authors to configure
various behaviours of tags and their attributes. This provides for an
extensible interface to implement any future tag / attribute related
plugin behaviour.

In place of the preRender hook, let's introduce the processNode plugin
hook, which taps directly into the merged html processing stage,
allowing a plugin to make modifications to any html node. This also
offers considerable performance benefits over pre / postRender, which
requires parses the entire page content.

The Plugin and PluginManager abstractions are also thus introduced, to
consolidate old and new Plugin related logic.
In place of the all encompassing functionalities in Page.js, let's link
together and utilise the new abstractions in Page.js.

The result is a few key 'manager' abstractions (PluginManager,
ExternalManager, LayoutManager, Site), each responsible for a distinct
entity (Layout, External, Plugin, Page), improving the readability of
the codebase.
Site conversion of github wiki files ties heavily to the separate files
(header, footer, ...) forming layouts.
For example, _Footer.md is translated into a MarkBind footer source
file.

With the new layout system, let's adapt this accordingly.
Github wiki files are extracted, then instead inserted into a default
site conversion template, which closely resembles the result of the
previous conversions.
@ang-zeyu
Copy link
Contributor Author

Merging this in a little earlier in case anyone starts looking at the backend.

*fingers crossed*, but we can find (hopefully) and fix any issues before release

@ang-zeyu ang-zeyu merged commit 741f30f into MarkBind:master Dec 12, 2020
Big Picture automation moved this from In progress to Completed Dec 12, 2020
ang-zeyu added a commit that referenced this pull request Jan 14, 2022
The processing flow revamp in #1403 to nunjucks->markdown->html
removes the need for ignoring nunjucks' {% ... %} inside
the markdown-it-attrs plugin manually.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment