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

Attempt to enable inclusion of partial markdown #227

Closed
wants to merge 13 commits into from

Conversation

ycmjason
Copy link
Contributor

@ycmjason ycmjason commented Apr 23, 2018

Very unstable, need more advice

I have attempted to add this functionality as discussed in #222.

Current implementation facilitates the below functionality:

  1. Replaces <!-- include ./path/to/my/file.md --> with the content of the file (recursively)
  2. Hot module reloading of dependencies

There are currently 2 known problems in this implementation:

  1. Do not utilise the cache properly, files are being read multiple times even when no content is changed. (Slow down the build process.)
    • Maybe I could create another cache in mdExplodeIncludes()
  2. <!-- include PATH --> could not be escaped even in code block. If PATH is an actual file, the file content would be used. This behaviour might not be desirable in some occasion. (e.g. when I was documenting about this functionality.)
    • Not sure how should I determine when to escape...

@ycmjason ycmjason mentioned this pull request Apr 23, 2018
@ycmjason ycmjason force-pushed the explode-md-includes branch from fff62f3 to 4416673 Compare April 23, 2018 21:10
Copy link
Member

@ulivz ulivz left a comment

Choose a reason for hiding this comment

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

LGTM, only some small things.

const deps = []

return {
explodedSrc: src.replace(/<!-- *include +(.*?) *-->/g, (match, path) => {
Copy link
Member

Choose a reason for hiding this comment

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

Using <!--\s*include\s+([^\s]+)\s*--> or <!-- include --> will be matched

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. 🎉

explodedSrc: src.replace(/<!-- *include +(.*?) *-->/g, (match, path) => {
try {
const absolutePath = resolve(cwd, path)
const content = readFileSync(absolutePath, 'utf8')
Copy link
Member

Choose a reason for hiding this comment

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

Using a unified convention at the same project. either that all are await readFile or all are readFileSync

Copy link
Contributor Author

@ycmjason ycmjason Apr 24, 2018

Choose a reason for hiding this comment

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

I have made this loader asynchronous. And it took me a really really long time to complete the asyncReplace function. I am so glad that I did this in such an elegant way.

Also it took me really long to debug the async loader as I forgot to call the callback when the loader hit the cache. 😭

Anyhow, it is using readFile now.


const cache = LRU({ max: 1000 })
const devCache = LRU({ max: 1000 })

module.exports = function (src) {
const file = this.resourcePath
const cwd = path.dirname(file)
Copy link
Member

Choose a reason for hiding this comment

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

just leverage dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain more on this?

@@ -0,0 +1,49 @@
const { promisify } = require('util')
Copy link
Member

Choose a reason for hiding this comment

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

You can use fs-extra here without having to promisify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, didn't know it exists. I will do this now.

@ycmjason
Copy link
Contributor Author

ycmjason commented Apr 24, 2018

@yyx990803 @ulivz
I would need more advice from you guys as mentioned in the main thread. 😀

Do you guys think those are big concerns?

Furthermore, one more thing to consider, should we force every partial to end with .partial.md and avoid vuepress from rendering a page for it? As discussed in #222.

@ycmjason ycmjason force-pushed the explode-md-includes branch 2 times, most recently from 199f99e to d1c0da8 Compare April 27, 2018 20:27
@ycmjason ycmjason force-pushed the explode-md-includes branch from d1c0da8 to 961eada Compare April 27, 2018 20:28
module.exports = async function (src) {
const cb = this.async()
try {
/* IMPORTANT: I didn't indent these lines to hopefully get a better looking diff */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have un-indented the lines in the try { ... } catch block to show a better looking diff. Please re-indent when PR is accepted. 😊

@ycmjason
Copy link
Contributor Author

Regarding the caching problem, after some think about it, it is probably not a problem because a partial is expected to be part of a longer markdown, therefore each partial should be included only once. 🌮

@kevinmpowell
Copy link

Any idea on when this is going to be released? I'm working on a team that could really use this functionality.

@ycmjason
Copy link
Contributor Author

ycmjason commented Jun 5, 2018

@kevinmpowell
You can find the roadmap here. #421

Hopefully this will get released in 1.0.

@ycmjason ycmjason force-pushed the explode-md-includes branch from 51da635 to 63d755e Compare June 6, 2018 16:58
@ulivz ulivz force-pushed the master branch 3 times, most recently from c992e38 to c2eaff3 Compare July 12, 2018 18:01
@ulivz ulivz force-pushed the master branch 4 times, most recently from 0c3bc3a to cf1e6fc Compare July 28, 2018 08:12
@ycmjason
Copy link
Contributor Author

ycmjason commented Aug 8, 2018

@davemacdo @MartinMuzatko
Hello, I just came back from holidays. And I think you made a legit point. I personally quite like the syntax:

<<[path/to/file.md]

But I guess I will not make the changes to this PR unless @ulivz agree to it. 👍

@ulivz ulivz removed the 1.0.0 label Aug 20, 2018
@ulivz ulivz force-pushed the master branch 2 times, most recently from bd69d41 to 903138e Compare October 3, 2018 22:21
@songololo
Copy link

Out of curiosity - how would partial markdown files work in regards to front-matter. Would each partial share access to the parent markdown's front-matter, or would each partial have its own front-matter?

I like the latter, because then it would be possible to have a separate markdown file for each method in package documentation, such that it is possible to define (in the each partial's front matter) the method name, parameter names and types, return names and types, etc, then render to an automated vue component. This would make for much cleaner and easier to read documentation code that could then also be automatically parsed from raw code, e.g. python docstrings, into markdown docs...

@ulivz ulivz force-pushed the master branch 2 times, most recently from 3f79224 to fb05066 Compare October 23, 2018 15:09
@ulivz ulivz force-pushed the master branch 3 times, most recently from 6c3127f to 71574f2 Compare December 18, 2018 18:27
@ulivz ulivz force-pushed the master branch 5 times, most recently from 316e022 to 1284944 Compare January 29, 2019 11:47
@dovy
Copy link

dovy commented Mar 16, 2019

Any progress on this?

@timaschew
Copy link
Contributor

@ycmjason

Since there is a similar syntax already to include code snippets which is

<<< @/filepath

it would be nice to have something similar to include markdown files.

But since v1 is not stable yet this can also be changed later (in another PR), right?

In general I like the approach of using a syntax which works when the markdown is rendered also on GitHub. It allows really easy contribution. Here is an example:

  1. Open this page: https://docs.microsoft.com/en-us/azure/virtual-machines/linux/tutorial-manage-vm
  2. Click on the Edit button
  3. Spot the [!INCLUDE cloud-shell-try-it.md] in the rendered markdown. You can click on it and you'll be able to edit that file.

The syntax they use is:

[!INCLUDE [cloud-shell-try-it.md](../../../includes/cloud-shell-try-it.md)]

related: #1474

@timaschew
Copy link
Contributor

Does the current implementation allow to include markdown files which itself have references to other resources, like links to other markdown files and images (or even nested includes)?

@aliniacb
Copy link

We can use the markdown-it-include plugin to achieve this. The only issue is the rebuild process. I have opened an issue here

@ycmjason ycmjason closed this May 29, 2019
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.