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

Adds comment syntax to Markdoc #198

Merged
merged 4 commits into from Sep 20, 2022
Merged

Adds comment syntax to Markdoc #198

merged 4 commits into from Sep 20, 2022

Conversation

rpaul-stripe
Copy link
Contributor

This PR introduces support for comments in Markdoc. Some users (including Stripe) have previously relied on a no-op {% comment %} tag to be able to include non-rendering content in a document, but the content inside of the tag is still subject to parsing and validation, which creates some undesirable side effects, particularly when trying to comment out portions of a doc that contain other tags or malformed tags.

Rather than special-casing the behavior of the {% comment %} tag in a way that would undermine the consistency of tag semantics, we instead decided that Markdoc comments will use the standard HTML comment syntax (<!-- -->). This syntax is already supported for commenting in many CommonMark-compliant parsers, via broader support for arbitrary HTML markup inside of Markdown.

This PR adds a new plugin to MarkdownIt that just adds that comment syntax, making it possible to have HTML comments without having to enable general HTML parsing. The resulting comments are incorporated into the AST but are omitted from the rendered document (they do not get propagated through as HTML comments in the output). Users can override the schema definition for comment nodes and implement their own transform function if they would like different behavior, though we do not have support for rendering actual HTML comments via the render tree.

Things included in this PR:

  • A MarkdownIt plugin that introduces block and inline parser rules for Markdoc comments
  • A new document node schema type for comments
  • Schema updates to allow comment nodes at the document level and in inlines (I think this is all we need, but I may have missed something here)
  • Test cases for the parser and for the MarkdownIt plugin
  • Marktest cases to ensure that comments parse and do not render as expected
  • A tweak to the parser code to make sure that the content of the comment gets applied to the AST node
  • A new tokenizer option for turning on the plugin
  • An assortment of unrelated marktest cases that I wrote to test various things recently and rolled into this PR

Copy link
Contributor

@mfix-stripe mfix-stripe left a comment

Choose a reason for hiding this comment

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

This is great! Just have one question around whether we should make allowComments default true. PTAL @rpaul-stripe

const tokenizer = new markdoc.Tokenizer({ allowIndentation: true });
const tokenizer = new markdoc.Tokenizer({
allowIndentation: true,
allowComments: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels to me like allowComments should be true by default, no?

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 do intend for it to eventually be on by default, but for the moment I want to keep it to off until we've had an opportunity to test it in practice more comprehensively.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh. Makes sense.

type: 'document',
children: [
{ type: 'paragraph' },
{ type: 'comment', attributes: { content: 'foo' } },
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat!

@@ -45,6 +45,7 @@ function handleAttrs(token: Token, type: string) {
}
case 'text':
case 'code':
case 'comment':
Copy link
Contributor

Choose a reason for hiding this comment

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

Love how this part of the code is working.

config: MarkdownIt.Options & { allowIndentation?: boolean } = {}
config: MarkdownIt.Options & {
allowIndentation?: boolean;
allowComments?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Default to true?

const example = parse(`
this is a test

<!-- example comment -->
Copy link
Contributor

Choose a reason for hiding this comment

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

For these last few tests, should we include the foo after the comments just to ensure it keeps the content after a multi-line comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a good suggestion, adding it to the tests caught a bug. I'll have a push fixed shortly.

import type StateBlock from 'markdown-it/lib/rules_block/state_block';
import type StateInline from 'markdown-it/lib/rules_inline/state_inline';

const OPEN = '<!--';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm totally peaceful with this approach, but I wonder if we should use the same regex that markdown-it uses internally: https://github.com/markdown-it/markdown-it/blob/df4607f1d4d4be7fdc32e71c04109aea8cc373fa/lib/common/html_re.js#L18

var comment     = '<!---->|<!--(?:-?[^>-])(?:-?[^-])*-->';

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'd prefer to avoid using a regex here

src/tokenizer/plugins/comments.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mfix-stripe mfix-stripe left a comment

Choose a reason for hiding this comment

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

Approving to unblock, but feel free to address the other comments and rerequest review 👍

@rpaul-stripe rpaul-stripe merged commit a18ffcc into main Sep 20, 2022
@rpaul-stripe rpaul-stripe deleted the rpaul/comments branch September 20, 2022 13:03
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.

None yet

2 participants