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.

Sign up for GitHub

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

Jump to bottom

Fix multi-line comment bug #3

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix multi-line comment bug #3

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
51f4ca3
proof of multiline comment bug
slorber Jun 22, 2023
457a2f8
fix multi-line comments edge case
slorber Jun 22, 2023
5b69693
add "blank line" tests
slorber Jun 22, 2023
f496b9d
Add recommended fix from Titus
slorber Jun 23, 2023
426dde7
test with dev assertions
slorber Jun 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ function tokenize(effects, ok, nok) {
return nok(code)
}

if (markdownLineEnding(code)) {
return atLineEnding(code);
}

Comment on lines +112 to +115
Copy link
Author

Choose a reason for hiding this comment

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

This is the fix.

Not sure what I'm doing 🤪
But I assume we shouldn't effects.enter(types.data) if there's no data to consume on that line

Copy link

Choose a reason for hiding this comment

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

blank lines (only occurs in the block/flow version) probably are broken from a quick glance. Otherwise looks good. See also https://github.com/micromark/micromark/blob/e10f892185d5616db6a9efad3a557ca1845d1843/packages/micromark-core-commonmark/dev/lib/html-text.js#L129

Copy link
Author

Choose a reason for hiding this comment

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

Thanks 👍

Not sure what you mean by "blank lines" 🤪 do you have an example I could use in a test?

Copy link

@wooorm wooorm Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

<!--\n\n-->, <!--\na\n\nb\n-->

Copy link
Author

Choose a reason for hiding this comment

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

I tried these samples in tests and not sure to understand what you mean by "probably are broken". What do you think it the bad behavior these samples produce?

According to my local tests:

  • Those comments are removed when they should
  • Those comments are serialized back in their original form when they should

Just added those to the test in case you want to take a look.

Copy link

Choose a reason for hiding this comment

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

Might have to run the tests with --conditions development to get instrumented code that checks if the extension works (so node test.mjs -> node --conditions development test.mjs). That’s not loaded normally because it would slow everyone down and increase the bundle size. See also: https://github.com/micromark/micromark#size--debug

My hunch is that there are empty tokens, which should not exist.

If that works, it’s all good.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'm running DEBUG="*" node --conditions development test/test.mjs and seeing the debug statements now 👍

The current code (even before my changes) has this assertion error at the comment end step:

Assertion: expected last token to be open. code=62

It looks like it does not like to consume just after an exit:

if (code === codes.greaterThan) {
  effects.exit(types.data);
  effects.consume(code);
  effects.exit("comment");
  return ok(code);
}

I'm not sure to understand the issue, that looks fine to me 🤔

Anyway, if I do this (which kind of feel useless?), now both tests and assertions are all passing:

if (code === codes.greaterThan) {
  effects.exit(types.data);
  effects.enter("commentEnd"); // NEW
  effects.consume(code);
  effects.exit("commentEnd"); // NEW
  effects.exit("comment");
  return ok(code);
}

Does it make sense to add the code above?

My hunch is that there are empty tokens, which should not exist.

Not sure how I can see those empty tokens. If there are no more assertion failures, does it means there are no empty tokens?

Copy link

Choose a reason for hiding this comment

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

FYI this project we‘re discussing on has exactly one commit. It’s very likely that it doesn’t work well, is not used a lot in practise, and might be abandoned.

I'm not sure to understand the issue, that looks fine to me 🤔

It’s not. That’s why there’s an error: every byte has to be in something specific.

Anyway, if I do this (which kind of feel useless?), now both tests and assertions are all passing:

Yep, that’s good! That’s the important part: putting every byte into something. For remark, which has ASTs, that’s indeed useless. But micromark can be used to make CSTs, where every character is present.

Not sure how I can see those empty tokens. If there are no more assertion failures, does it means there are no empty tokens?

If there are no more errors, including for those blank line fixtures (<!--\n\n\n-->), it’s good! 👍

Copy link
Author

Choose a reason for hiding this comment

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

If there are no more errors, including for those blank line fixtures (), it’s good! 👍

Thanks!

FYI this project we‘re discussing on has exactly one commit. It’s very likely that it doesn’t work well, is not used a lot in practise, and might be abandoned.

Yes I understand that, and as I wasn't sure Lee would answer/merge fast I just published @slorber/remark-comment with these PR fixes.

We'll use it on Docusaurus mostly to make the migration easier, according to what I see it seems good enough as a transitory measure. We have a flag for users to opt-out of this plugin once they have fully migrated to MDX comments.

effects.enter(types.data)

if (code === codes.dash) {
Expand Down
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

58 changes: 55 additions & 3 deletions test/test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ assert.equal(

<!-- has a comment -->

<!--
has a multi-line comment
-->

<!-- another
multi-line
comment -->

and a paragraph
`
),
Expand All @@ -52,12 +60,32 @@ assert.equal(

<!-- has a comment -->

<!--
has a multi-line comment
-->

<!-- another
multi-line
comment -->

and a paragraph
`,
{ ast: true }
),
'# This <!-- inline -->document\n\n<!-- has a comment -->\n\nand a paragraph\n'
)
`# This <!-- inline -->document

<!-- has a comment -->

<!--
has a multi-line comment
-->

<!-- another
multi-line
comment -->
Comment on lines +87 to +93
Copy link
Author

Choose a reason for hiding this comment

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

Apart the refactor to a template literal, only those lines were added and the rest remains unchanged


and a paragraph
`)

// It renders to HTML via Rehype
assert.equal(
Expand All @@ -66,6 +94,14 @@ assert.equal(

<!-- has a comment -->

<!--
has a multi-line comment
-->

<!-- another
multi-line
comment -->

and a paragraph
`
),
Expand All @@ -80,11 +116,19 @@ assert.equal(

<!-- has a comment -->

<!--
has a multi-line comment
-->

<!-- another
multi-line
comment -->

and a paragraph
`,
{ ast: true }
),
'<h1>This document</h1>\n\n<p>and a paragraph</p>'
'<h1>This document</h1>\n\n\n\n<p>and a paragraph</p>'
Copy link
Author

Choose a reason for hiding this comment

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

See your own test comment: the extra line breaks are expected

)

// It renders to HTML via Micromark
Expand All @@ -94,6 +138,14 @@ assert.equal(

<!-- has a comment -->

<!--
has a multi-line comment
-->

<!-- another
multi-line
comment -->

and a paragraph
`
),
Expand Down