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

Update: Upgrade remark-parse to v7 (fixes #77, fixes #78) #175

Merged
merged 1 commit into from
Apr 25, 2021
Merged

Conversation

btmills
Copy link
Member

@btmills btmills commented Feb 27, 2021

remark-parse@7.0.0 includes remarkjs/remark#423, which fixes handling of leading and trailing newlines in fenced code blocks.

For reviewers:

  1. remark-parse v6, v7, and v8 were semver-major fixes. v9 was a full rewrite. Are we comfortable with a parser major version bump in a semver-minor release? This is currently tagged as Update: so it's semver-minor rather than semver-patch.
  2. If so, how far? This PR bumps to v7 because it fixes two known issues, First empty line in code fragment breaks line numeration in error messages #77 and First empty line in code fragment silents rules about first empty line in code #78. I'm not aware of any eslint-plugin-markdown issues that would be fixed by v8 or v9.
  3. If we're fine upgrading past v7 for speculative parsing accuracy improvements that don't knowingly affect eslint-plugin-markdown, should we stop at v8 or go all the way to the re-written parser in v9? If the latter, I'll close this in favor of Chore: upgrade remark-parse to 9.0.0 #171.

Changelogs:

`remark-parse@7.0.0` includes
remarkjs/remark#423, which fixes handling of
leading and trailing newlines in fenced code blocks.

Tagging this as `Update` so it'll be semver-minor. Are we comfortable
with a parser major version bump in a semver-minor release?
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM

@btmills
Copy link
Member Author

btmills commented Apr 5, 2021

Thanks for the review @nzakas. What are your thoughts on those three follow-up questions in the description? I at least want v7 to fix these bugs, but I could use some input on v8 or v9.

@nzakas
Copy link
Member

nzakas commented Apr 6, 2021

Ah sorry, totally breezed over those. My vote would be to just do the minimum to fix the issues we were having right now (stop the bleeding). We can always do further upgrades later but I’d rather get a safe fix out first.

@remcohaszing
Copy link

All remark-parse does is make mdast-util-from-markdown work with unified.

Since eslint-plugin-markdown doesn’t really use unified features, the following changes will yield the same result with fewer dependencies and a properly typed ast.

- const unified = require("unified");
- const remarkParse = require("remark-parse");
+ const fromMarkdown = require("mdast-util-from-markdown");
- const markdown = unified().use(remarkParse);
- 
  /**
  function preprocess(text, filename) {
-     const ast = markdown.parse(text);
+     const ast = fromMarkdown(text);
      const blocks = [];

@btmills
Copy link
Member Author

btmills commented Apr 25, 2021

Thanks for the tip @remcohaszing. It'd be nice to reduce the size of the dependency tree, so I gave that a shot, but it has the same change to indents that I pointed out in #171 (comment). That makes using remark-parse v9 or the underling mdast-util-from-markdown parser a non-trivial change, but I'd welcome the research if someone wanted to figure out what would be required.

@btmills btmills merged commit f1e153b into main Apr 25, 2021
@btmills btmills deleted the remark-v7 branch April 25, 2021 20:57
btmills added a commit that referenced this pull request May 20, 2021
The previous parser, `remark-parse` v7, included a transitive dependency
on an npm package with a security vulnerability. Newer versions of
`remark-parse` are wrappers around a new underlying parser,
`mdast-util-from-markdown`, so we can use that directly.

The previous parser also failed to preserve `\r\n` line endings,
replacing them all with `\n`. The new parser correctly preserves `\r\n`
line endings, finally providing a fix for the failing test case I
cherry-picked in the previous commit. The improved behavior also
uncovered an incorrect line ending test assertion that this commit
corrects.

While this change is in theory fully compatible, containing just bug
fixes, I'm tagging it `Update:` in case there are compatibility changes
in the new parser. This is consistent with #175, which upgraded
`remark-parse` v5 to v7 in a semver-minor `Update:` change.
btmills added a commit that referenced this pull request May 26, 2021
* Docs: Fix typo in comment

* Docs: Add RangeMap JSDoc property descriptions

* Fix: Use internally-calculated code block trim lengths

Future versions of the Markdown processor don't include per-line indent
information. Thankfully, we're already calculating it in
`getBlockRangeMap()` because we need it to correctly map fix ranges for
lines that are under-indented to the left of their opening code fence.
This includes the indent in the `RangeMap` for each line and uses that
in `adjustBlock()` instead.

This uncovered an incorrect assertion in one of the tests, which
combined an indented code block, a message on a line with mixed spaces
and tabs indentation, and an under-indented final line. The message
column was incorrectly asserted as being in the whitespace indentation
before the line.

* Chore: Test multline autofix with CRLF

Originally discovered in #120 and cherry-picked from #125.

* Update: Replace Markdown parser (fixes #125, fixes #186)

The previous parser, `remark-parse` v7, included a transitive dependency
on an npm package with a security vulnerability. Newer versions of
`remark-parse` are wrappers around a new underlying parser,
`mdast-util-from-markdown`, so we can use that directly.

The previous parser also failed to preserve `\r\n` line endings,
replacing them all with `\n`. The new parser correctly preserves `\r\n`
line endings, finally providing a fix for the failing test case I
cherry-picked in the previous commit. The improved behavior also
uncovered an incorrect line ending test assertion that this commit
corrects.

While this change is in theory fully compatible, containing just bug
fixes, I'm tagging it `Update:` in case there are compatibility changes
in the new parser. This is consistent with #175, which upgraded
`remark-parse` v5 to v7 in a semver-minor `Update:` change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants