Add eslint and rules to Marked

[joshbruce]

See #991

While Marked is not exceptionally large. There are some directions that the community would like to take the codebase where eslint would probably be helpful. Further, linting might be able to help some regression checks that are not related to input and output.

Question to the community is what rules make sense - and is eslint the proper library to use? Or, is this proposal just completely insane and need to go away?

[smhg]

I guess going for standard makes a lot of sense? I personally prefer semistandard, but GitHub stars don't lie.

[joshbruce]

@smhg: Nice! Never heard of these before. Seems the only difference between the two is on whether to semicolon or not, yeah?

While I don't care either way regarding semicolons I do have a question, given what I understand JS to be (intepretated language) and how the browser performs its interpretation:

If minified (no unnecessary whitespace), how does the browser's JS engine know when it's hit the end of line? Python uses the whitespae. PHP uses the semicolon and curly brackets. Swift uses the whitespace (if I understand the compiler properly). And so on.

Honestly curious, because I didn't know it was possible in JS.

[smhg]

The only difference are the semicolons indeed.

Standard relies on ESLint, and is also usable as an ESLint plugin. So npm i standard and npm i eslint eslint-config-standard eslint-plugin-standard are more or less equal (someone please confirm).

I actually use the latter as this allows me to override the no-semicolons setting. This way I avoid the in-between semistandard dependency (which might have its own update pace).

I mention the above because that might be a nice entry path: start with eslint + standard (aka the second option above) and disable all rules that result in warnings/errors. Then remove disabled rules one-by-one as you/others find time to make the rule pass.

Additionally: ESLint can fix quite a lot of warnings/errors itself with the --fix option. So there are probably some quick wins.

Should there be disagreements between contributors about the used style, I think Prettier can be an automated solution. I have no real experience with it, so I hope someone else can step in.

[smhg]

About no-semicolons and minifying: good question. I gave it a try and uglify-js adds semicolons where necessary when removing whitespace.

These are 2 separate concerns in the end: the standard style and (a part of) linting apply formatting rules to optimize your and others' workflow. While minification optimizes for delivery/execution.

[Feder1co5oave]

I do think a linter could help keeping a steady code style. As the catchphrase of standard says:

Save precious code review time by eliminating back-and-forth between reviewer & contributor.

I just tried out semistandard. Since it complains a lot about indentation, and marked.js is unindented by 2 spaces on all lines to save horizontal space, a lot of warning were issued.

tl;dr -- scroll to the end

Trying to automatically fix trivial stuff, I ran standard --fix. A lot of warning came up, again. Few examples:

/home/federico/marked/lib/marked.js:33:40: Unexpected space between function name and paren.
/home/federico/marked/lib/marked.js:34:3: Unexpected newline between function and ( of function call.

refers to this:

block.list = replace(block.list)
  (/bull/g, block.bullet)
  ('hr', '\\n+(?=\\1?(?:[-*_] *){3,}(?:\\n+|$))')
  ('def', '\\n+(?=' + block.def.source + ')')
  ();

I guess we could re-think the replace function to be more read-friendly.

/home/federico/marked/lib/marked.js:448:25: Unnecessary escape character: \[

refers to this:

  text: /^[\s\S]+?(?=[\\<!\[_*`]| {2,}\n|$)/

because opening braces don't need to be escaped inside character classes (but I think it doesn't hurt).

/home/federico/marked/lib/marked.js:568:11: Expected a conditional expression and instead saw an assignment.

refers to this:

if (cap = this.rules.escape.exec(src)) {

which I guess could be written as

cap = this.rules.escape.exec(src)
if (cap) {

/home/federico/marked/lib/marked.js:951:5: Return statement should not contain assignment.

refers to

Parser.prototype.next = function() {
  return this.token = this.tokens.pop();
};

which can be easily split into separate statements too. This was the "useless" part in my opinion. It then complained about a few multiple var declaration-initializations, like

var out = ''
    , link
    , text
    , href
    , cap;

Unused variables, already-defined ones, or possibly undefined ones. Warnings aside, semistandard automatically rearranges code to make it comply it its predefined coding style. For example:

@@ -357,8 +357,8 @@ Lexer.prototype.token = function(src, top, bq) {
           type: this.options.sanitize
             ? 'paragraph'
             : 'html',
-        pre: !this.options.sanitizer
-          && (cap[1] === 'pre' || cap[1] === 'script' || cap[1] === 'style'),
+          pre: !this.options.sanitizer &&
+          (cap[1] === 'pre' || cap[1] === 'script' || cap[1] === 'style'),
           text: cap[0]
         });
         continue;
@@ -518,7 +518,7 @@ function InlineLexer(links, options) {
     this.options = options || marked.defaults;
     this.links = links;
     this.rules = inline.normal;
-  this.renderer = this.options.renderer || new Renderer;
+    this.renderer = this.options.renderer || new Renderer();
     this.renderer.options = this.options;

     if (!this.links) {
@@ -736,10 +736,10 @@ InlineLexer.prototype.smartypants = function(text) {

   InlineLexer.prototype.mangle = function (text) {
     if (!this.options.mangle) return text;
-  var out = ''
-    , l = text.length
-    , i = 0
-    , ch;
+    var out = '',
+      l = text.length,
+      i = 0,
+      ch;

     for (; i < l; i++) {
       ch = text.charCodeAt(i);
@@ -770,11 +770,11 @@ Renderer.prototype.code = function(code, lang, escaped) {
     }

     if (!lang) {
-    return '<pre><code>'
-      + (escaped ? code : escape(code, true))
-      + '\n</code></pre>';
+      return '<pre><code>' +
+      (escaped ? code : escape(code, true)) +
+      '\n</code></pre>';
     } // this looks WORSE to me

tl;dr

Overall, there were a few useful warnings, some pieces of code could be re-written, readability doesn't improve (in some places, it got worse IMO). Applying this linter means we will have to commit a huge diff to the codebase (mainly concerning whitespace), and that could lead to many more conflicts in the future than if we didn't.

I'd be glad to hear your opinions on this topic. Maybe we should fall back to have eslint with custom rules (I don't know how to do that)? Or have no linter at all?

[joshbruce]

@Feder1co5oave: Is it possible turn off some of the rules; some of @smhg's comments imply this possibility. Regarding creating a common style - some talking points:

1. Re horizontal space consevation - is this really necessary - thinking of the minification. What developer/user value is received by this...or am I not understanding the point being made entirely?
2. Do most of the contributors use an editor that understands/respects .editorconfig? Might want to do both. That way the editors themselves are automatically using some of the rules.
3. I do not personally have a preference on which tool; having said that, if there is a standard (or spec) that is recommended by the JS community, think we should use it. Kinda like PHP has PSR-1 and PSR-2.

[smhg]

@Feder1co5oave to tackle your very valid concerns: I think the approach I suggested solves this.

It would come down to starting with: npm i eslint eslint-plugin-node eslint-config-standard eslint-plugin-standard

Plus adding an .eslintrc.json:

{
  "extends": "standard",
  "plugins": [
    "standard"
  ],
  "rules": {
    "semi": "off"
  }
}

Then run ESLint on the source and for every warning/error you'll get the rule that caused it (it seems like standard doesn't do that, so this is a reason to use ESLint directly). Add all those rules to the rules section in the above .eslintrc.json just like the example semi rule, turning them off.

You could then, commit-by-commit, when someone feels up to it, turn on a rule when you fix it.

[joshbruce]

For what it's worth, I also work government projects. The folks who created the US Web Design System recommend AirBnB.

https://www.npmjs.com/package/eslint-config-airbnb-base

https://github.com/18F/web-design-standards/issues/2310

??

[smhg]

Also, the --init option to ESLint lets you choose between Google, Airbnb or Standard:

node_modules/.bin/eslint --init

[styfle]

Wasn't this fixed in #1020 already?