Update build process for ES6 #320
Conversation
* Make npm run build run npm compile (it needs the output) * Switch to ESlint so we can actually use ES6 without the linter crying.
| @@ -7,12 +7,12 @@ | |||
| "test": "istanbul cover --report cobertura --config .istanbul.yml -i \"lib/**/*.js\" jasmine-node -- spec --verbose --junitreport --captureExceptions", | |||
| "check": "jasmine-node spec --verbose --junitreport --captureExceptions", | |||
| "gendoc": "jsdoc -r lib -P package.json -R README.md -d .jsdoc", | |||
| "compile": "babel -s -d lib src", | |||
| "compile": "npm run lint && babel -s -d lib src", | |||
There was a problem hiding this comment.
is there a reason to maintain a separate compile target? why not just stick it into build ?
| ignoreComments: true, | ||
| }], | ||
| curly: ["error", "multi-line"], | ||
| } |
There was a problem hiding this comment.
should we enable http://eslint.org/docs/rules/require-jsdoc and http://eslint.org/docs/rules/valid-jsdoc? It would be a shame if we started slipping into bad habits with the jsdoc.
There was a problem hiding this comment.
Interestingly, turning these on produces 537 warnings, so presumably it's somewhat stricter than jshint :/
Should we turn them on and aspire to fix the warnings?
To allow things we've been OK about previously
|
ptal |
| "dist": "npm run build", | ||
| "watch": "watchify --exclude olm browser-index.js -o dist/browser-matrix-dev.js -v", | ||
| "lint": "jshint -c .jshint src spec && gjslint --unix_mode --disable 0131,0211,0200,0222,0212 --max_line_length 90 -r spec/ -r src/", | ||
| "lint": "eslint src", |
There was a problem hiding this comment.
shouldn't we check the spec folder too? we used to, and jenkins.sh apparently does.
There was a problem hiding this comment.
can we set --max-warnings on this?
There was a problem hiding this comment.
fixed (and max-warnings now redundant since everything is an error)
| ignoreComments: true, | ||
| }], | ||
| curly: ["error", "multi-line"], | ||
| "require-jsdoc": ["warn", { |
There was a problem hiding this comment.
why are these warnings rather than errors?
| "dist": "npm run build", | ||
| "watch": "watchify --exclude olm browser-index.js -o dist/browser-matrix-dev.js -v", | ||
| "lint": "jshint -c .jshint src spec && gjslint --unix_mode --disable 0131,0211,0200,0222,0212 --max_line_length 90 -r spec/ -r src/", | ||
| "lint": "eslint src", | ||
| "prepublish": "npm run compile && git rev-parse HEAD > git-revision.txt" |
and make everything errors, so now you can do local builds with lint failures, but CI will fail and you can't release.
There was a problem hiding this comment.
I think we need to enable some rules for this to be useful.
https://github.com/google/eslint-config-google looks useful, given we purport to follow the google style.
Remove some we don't care about. Set some other ones we do care about but don't currently adhere to to warn. Set the max warnings threshold to the current number of warnings, so we don't introduce more of them. Fix a bunch of legit lint errors and add exceptions to various places in the test code that does funny things with 'this'.
Turn off / tweak some options from it. Fix a double-definition. Add an eslint config to the spec directory to tell it about the jasmine magic globals.
crying.