Comply with standard JS style

[Gozala]

We should empty Standard JS style going forward as it is quite popular has a lot of users and contributors.

[Gozala]

This work is semi-blocked by #1236 as I would like to do all the renaming before we go ahead with any kind of mass reformatting. That being said I still can do an exploratory work as it's unlikely to be straight forward task. I'll be updating this thread as I make progress on this.

[Gozala]

Alright standard with flow type annotations is not straight forward but seems possible:

1. You configure standard to use custom parser.
2. You can use babel-eslint parser with eslint-plugin-flowtype to parse files with flow extensions.

And there for following changes to package.json gets standard capable of parsing our code 🎆:

diff --git a/package.json b/package.json
index ff227eb..f340882 100644
--- a/package.json
+++ b/package.json
@@ -40,6 +40,7 @@
   },
   "devDependencies": {
     "babel-cli": "6.10.1",
+    "babel-eslint": "6.1.2",
     "babel-plugin-syntax-class-properties": "6.8.0",
     "babel-plugin-syntax-flow": "6.8.0",
     "babel-plugin-transform-class-properties": "6.10.2",
@@ -52,6 +53,7 @@
     "browserify": "13.0.0",
     "browserify-hmr": "0.3.1",
     "ecstatic": "0.8.0",
+    "eslint-plugin-flowtype": "2.7.1",
     "flow-bin": "0.28.0",
     "gulp": "3.9.1",
     "gulp-sequence": "0.4.5",
@@ -60,10 +62,47 @@
     "gulp-util": "3.0.7",
     "gulp-watch": "4.3.5",
     "hotify": "0.1.0",
+    "standard": "7.1.2",
     "vinyl-buffer": "1.0.0",
     "vinyl-source-stream": "1.1.0",
     "watchify": "3.7.0"
   },
+  "standard": {
+    "parser": "babel-eslint",
+    "plugins": [
+      "flowtype"
+    ],
+    "rules": {
+      "flowtype/define-flow-type": 1,
+      "flowtype/require-parameter-type": 1,
+      "flowtype/require-return-type": [
+        1,
+        "always",
+        {
+          "annotateUndefined": "never"
+        }
+      ],
+      "flowtype/space-after-type-colon": [
+        1,
+        "always"
+      ],
+      "flowtype/space-before-type-colon": [
+        1,
+        "never"
+      ],
+      "flowtype/type-id-match": [
+        1,
+        "^([A-Z][a-z0-9]+)+Type$"
+      ],
+      "flowtype/use-flow-type": 1,
+      "flowtype/valid-syntax": 1
+    },
+    "settings": {
+      "flowtype": {
+        "onlyFilesWithFlowAnnotation": false
+      }
+    }
+  },
   "contributors": [
     "The Browserhtml Project Developers"
   ],

[Gozala]

Now unfortunately standard src/browser.js --fix seems to not quite handle job properly:

1. It fixes only some things but keeps reporting about more issues.
2. It seems to do incorrect changes with comment blocks.
3. It does not optimize imports but keeps complaining about them.

I will try to resolve remaining issues manually on that file and see where we get from there.

[Gozala]

Unfortunately we've hit the difficult one gajus/eslint-plugin-flowtype#72 so even if we reformat everything standard will still complain about duplicate errors when type and value is imported from the same module with a separate imports.

I will look I there is a way to fix it, alternatively at least this case can be worked around.

[Gozala]

Alright turns out there is a workaround, we could configure duplicate import errors to be disabled. Unfortunately that requires using eslint (which is what standard uses under the hood) + eslint-config-standard with all the rules imposed by standard + overrides which brings us to following:

diff --git a/package.json b/package.json
index ff227eb..4325985 100644
--- a/package.json
+++ b/package.json
@@ -40,6 +40,7 @@
   },
   "devDependencies": {
     "babel-cli": "6.10.1",
+    "babel-eslint": "6.1.2",
     "babel-plugin-syntax-class-properties": "6.8.0",
     "babel-plugin-syntax-flow": "6.8.0",
     "babel-plugin-transform-class-properties": "6.10.2",
@@ -52,6 +53,11 @@
     "browserify": "13.0.0",
     "browserify-hmr": "0.3.1",
     "ecstatic": "0.8.0",
+    "eslint": "3.3.1",
+    "eslint-config-standard": "5.3.5",
+    "eslint-plugin-flowtype": "2.7.1",
+    "eslint-plugin-promise": "2.0.1",
+    "eslint-plugin-standard": "2.0.0",
     "flow-bin": "0.28.0",
     "gulp": "3.9.1",
     "gulp-sequence": "0.4.5",
@@ -60,10 +66,31 @@
     "gulp-util": "3.0.7",
     "gulp-watch": "4.3.5",
     "hotify": "0.1.0",
+    "standard": "7.1.2",
     "vinyl-buffer": "1.0.0",
     "vinyl-source-stream": "1.1.0",
     "watchify": "3.7.0"
   },
+  "standard": {
+    "parser": "babel-eslint",
+    "plugins": [
+      "flowtype"
+    ],
+    "rules": {
+      "flowtype/define-flow-type": 1,
+      "flowtype/space-before-type-colon": [
+        1,
+        "never"
+      ],
+      "flowtype/use-flow-type": 1,
+      "flowtype/valid-syntax": 1
+    },
+    "settings": {
+      "flowtype": {
+        "onlyFilesWithFlowAnnotation": false
+      }
+    }
+  },
   "contributors": [
     "The Browserhtml Project Developers"
   ],

and additions of .eslintrc file:

{
  "parser": "babel-eslint",
  "extends": "standard",
  "plugins": [
    "flowtype"
  ],
  "rules": {
      "flowtype/define-flow-type": 1,
      "flowtype/space-before-type-colon": [
          1,
          "never"
      ],
      "flowtype/use-flow-type": 1,
      "flowtype/valid-syntax": 1,
      "no-duplicate-imports": 0
  },
  "settings": {
      "flowtype": {
          "onlyFilesWithFlowAnnotation": false
      }
  }
}

With all this running eslint --fix src/browser.js still reports several issues, few of them was due to definitions that were never used which I removed manually, but I still see ✖ 51 problems and all of them are error ',' should be placed last comma-style.

Reformatting to move all commas to the end of lines does not appeal to me at all, so I'll try to see if I could use some tool to do that for us.

[Gozala]

Ok wrote a small JS formatter that uses babel/babylon parser to turn code into AST and [babel-generator (https://github.com/babel/babel/tree/master/packages/babel-generator) to generate JS code back from AST and exposed all this as tonic endpoint: https://tonicdev.com/gozala/57b62b7f61e23e1400e485e7

So I could reformat code as follows:

curl https://tonicdev.io/gozala/57b62b7f61e23e1400e485e7/branches/master -d "`cat ./src/browser.js`" > src/browser.frmt.js

Resulting file than can be fixed up via ESLint as follows:

eslint --fix src/browser.frmt.js

Unfortunately end result is not so great, as:

1. Tagged unions end up joined into single line
2. Arrow functions get formatted strangely.
3. Long arrays also are formatted as one liners.

I will take a look if there is some way to customize format of babel-generator or maybe someone even did a generator customization that I can reuse.

[Gozala]

Ok so I have being looking around and messing around with different tools and esformatter looks promising at least I got it to reformat code without issue 1, I'll have to play with it more to find out if it could actually be used to reformat all of our code.

[Gozala]

As someone pointed out on babel's slack channel it might be a better option to implement ESLint --fix that would cover comma-errors rather than other options I've considered. I have submitted an issue eslint/eslint#6941 & if someone will guide me and it will turn out to be not too large of a project I can pursue that route as well.

[Gozala]

Ok in the meantime we should at least enforce standard style on new files, so I opened #1240 to do so.

[Gozala]

Alright had bunch of conversation on different issues blocking this with maintainers of relevant projects. Created #1240 that workarounds some issues and enables standard for all new code. Have also created #1242 as an alternative way to gradually move code base in compliance with standard with a help of contributors. I'll close #1242 if I manage to automate the process.

[Gozala]

I feel like I'm falling into trap of thinking the writing my own code generator from AST would be easier than try to fix bugs I run into with esformatter. I'm starting to doubt if just going #1242 will be more effective.