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

extract markdown into an extension #7656

Merged
merged 33 commits into from
Jun 16, 2016
Merged

extract markdown into an extension #7656

merged 33 commits into from
Jun 16, 2016

Conversation

kieferrm
Copy link
Member

Extracts colorizing and previewing of markdown files into its own extension as described in #6106.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @egamma and @jrieken to be potential reviewers

}`;

let bodyClasses = {
remove: ['monaco-editor', 'vs', 'vs-dark', 'hc-black']
Copy link
Member

Choose a reason for hiding this comment

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

do we need monaco-editor?
and for the sake of being aligned and explicit should we call the theme classes: vscode-light, vscode-dark, and vscode-high-contrast?

@jrieken
Copy link
Member

jrieken commented Jun 14, 2016

@kieferrm We have some hygiene failure due to missing copy right statements

}
return `<pre class="hljs"><code><div>${md.utils.escapeHtml(str)}</div></code></pre>`;
}
}).use(mdnh, {});
Copy link

@cbreeden cbreeden Jun 14, 2016

Choose a reason for hiding this comment

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

If you don't mind me asking. As it is coded here, anytime a member wants to add a plugin to markdown-it, it seems they will have to:

  1. Npm install markdown-it-plugin --savedev in extensions folder
  2. Require package here and add package to .use(...)

This seems fine to me, but I believe that also means I would have to do this every time that extension is updated. I don't know of a good solution, but one solution I thought of is to allow for developers to create a "skeleton" package that simply exposes and api to the markdown extension that says use this. So people will be able to install a package, say: vscode-markdown-it-katex for math support which has a dependency on this package and it should just work. Not an ideal solution, but maybe there is a better one? I could open this as a feature request if you would like to defer this until another cycle. Or possibly a pull request later on.

Copy link
Member

Choose a reason for hiding this comment

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

@cbreeden your request is valid to me. People always need to customize the Markdown rendering process to meet their need. A final perfect solution is allowing users to load markdown-it anomynous extensions on the fly. But I don't find this feature documented on their website. So personaly I suggest:

  1. Find out or add on-the-fly loading mechanism for markdown-it. Maybe create a feature request on their repo.
  2. Emebed this feature into our seperate Markdown Extension. Provide users a configuration (an array is simple enough) to inject their markdown-it extensions written in JS.

Since we've already moved the markdown stuff out of VS Code, we may want to keep all related features in markdown or its dependencies instead of here.

If you are willing to help file these issues/requests to corresponding repositories, it would be really awesome. Otherwise I can do it myself.

Choose a reason for hiding this comment

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

Sounds great. Could you help me try to understand a little better how to implement this? I believe that .use is on the 'on-the-fly' as in it should work something like this:

var md = require('markdown-it')();
var mathPlugin = require('markdown-it-katex');

md.render('$1 \\over 2$'); // renders '$1 \\over 2$' in paragraph tag

md = md.use(mathPlugin);
md.render('$1 \\over 2$'); // renders '1/2' in new fancy math

So you could certainly iterate over a list of plugins to attach them. The issue is that I am not exactly sure how I would get the extension to obtain the mathPlugin object from a settings list. Also how to make this list persist through extension updates. Do you have any ideas?

Minor note: There are some other methods for extending markdown-it where you override the rendering functions to meet special needs. It's very similar to what vscode already has. But just getting the plugins to load would be great and worry about this later maybe.

Copy link
Member

Choose a reason for hiding this comment

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

When I say on-the-fly, it's more or less loading anomynous plugins:

plugins.each((ext) => {
  md.use(ext);
})

This is what we need to figure out (I have no idea about this either for now).

Updating is another topic. One solution is keeping our hands clean by only allowing users to inject javascript files and let users update the plugins themselves. Otherwise we need careful design about plugin upgrade.

Copy link
Member

Choose a reason for hiding this comment

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

For markdown-it plugins I suggest that we explore a path via settings. For instance, a user can manually configure additional plugins to use in settings.json like markdown.plugins: ["p1", "p2"]. We would try to find and configure them, but not install them.

@jrieken
Copy link
Member

jrieken commented Jun 15, 2016

@aeschli There is an integration test failure (https://travis-ci.org/Microsoft/vscode/jobs/137741881#L2247) with those changes. I believe this is cos we have added new token types etc and I guess you know for sure and how to fix it.

@jrieken jrieken merged commit f5898ac into master Jun 16, 2016
@jrieken jrieken deleted the kieferrm/markdown branch June 16, 2016 07:23
@cbreeden cbreeden mentioned this pull request Jun 16, 2016
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants