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 remark #62

Merged
merged 2 commits into from
Apr 29, 2017
Merged

Update remark #62

merged 2 commits into from
Apr 29, 2017

Conversation

wooorm
Copy link
Contributor

@wooorm wooorm commented Apr 26, 2017

Hi folks! 👋

I’ve updated remark! This does come with a change in HTML comments, so I had to use a loop around the getComments functionality.

I also removed remark directly. Rather, I added unified and remark-parse, which is a bit smaller, meaning faster installs.

Let me know if I should fix-up some code-style!

@jsf-clabot
Copy link

jsf-clabot commented Apr 26, 2017

CLA assistant check
All committers have signed the CLA.

@btmills
Copy link
Member

btmills commented Apr 28, 2017

@wooorm thanks for this, and thanks for your work on remark!

Previously getComments would parse the html AST node that was passed to it. Now it looks like remark appears to split comments into distinct nodes - is that correct? If so, can I remove the parse5 step and just strip the <!-- and --> from the node's value?

@wooorm
Copy link
Contributor Author

wooorm commented Apr 28, 2017

That’s correct, the following markdown:

<!--some eslint comment-->
<!--one more eslint comment-->
~~~js
code();
~~~

...is turned into:

{
  "type": "root",
  "children": [
    {
      "type": "html",
      "value": "<!--some eslint comment-->"
    },
    {
      "type": "html",
      "value": "<!--one more eslint comment-->"
    },
    {
      "type": "code",
      "lang": "js",
      "value": "code();"
    }
  ]
}

I’ll update the PR somewhere today!

@wooorm
Copy link
Contributor Author

wooorm commented Apr 29, 2017

Done! Let me know if there’s some code-style I need to fix!

@platinumazure
Copy link
Member

Do we want to support the line comments as well? (e.g., // eslint-disable-next-line someRule)

Probably a better question for @btmills, anyway.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

@wooorm this is great, thank you!

I just tested, and the dependency changes in this PR result in a 27% reduction in node_modules size for a production install, down to 1.1MB from 1.5MB 🎉

@platinumazure that's a good point, right now the transform doesn't support eslint-disable-next-line since it has to be a line comment. That would be a separate change from this PR, so I'll merge without it, but it's worth considering whether the plugin needs to support eslint-disable-next-line in an HTML comment.

@btmills btmills merged commit d2f2962 into eslint:master Apr 29, 2017
@wooorm
Copy link
Contributor Author

wooorm commented Apr 29, 2017

Awesome! 👍

@wooorm wooorm deleted the update-remark branch April 29, 2017 21:54
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.

4 participants