Skip to content
This repository has been archived by the owner on May 4, 2022. It is now read-only.

Issue #582: Add ESLint to package.json test script #586

Merged
merged 3 commits into from
Mar 17, 2017
Merged

Issue #582: Add ESLint to package.json test script #586

merged 3 commits into from
Mar 17, 2017

Conversation

ChristianMurphy
Copy link
Contributor

'posttest' automatically runs after 'npm test'

'posttest' automatically runs after 'npm test'
vertein
vertein previously approved these changes Mar 16, 2017
Copy link
Contributor

@vertein vertein left a comment

Choose a reason for hiding this comment

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

I like it. We have some merge conflicts now.

@ChristianMurphy
Copy link
Contributor Author

@vertein Resolved merge conflicts

@ChristianMurphy
Copy link
Contributor Author

ChristianMurphy commented Mar 16, 2017

📓 Note that the Google styleguide recommends some ES6 features be preferred over their ES5 counterparts.
If you do not want to drop ES5 support or add a transpiler the ES6 rules should be disabled. 🚫

apetro
apetro previously approved these changes Mar 16, 2017
Copy link
Contributor

@apetro apetro left a comment

Choose a reason for hiding this comment

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

I admit to a lack of expertise in ES best style practices.

I nonetheless passionately believe that we should just adopt whatever Google Style is, and do so with a minimum of exceptions.

@ChristianMurphy
Copy link
Contributor Author

ChristianMurphy commented Mar 16, 2017

The difference from a project standpoint is browser compatibility.
These are the feature Google code styleguide recommends, and how they are supported in mainline browsers.

Browser Arrow Functions Let/Const Rest/Spread Generator Functions/Yield
Chrome 57
Firefox 52
Edge 13
Safari 10
IE 11

Source: https://kangax.github.io/compat-table/es6/ ◀️ also lists support in beta and some legacy browsers.

Copy link
Contributor

@thevoiceofzeke thevoiceofzeke left a comment

Choose a reason for hiding this comment

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

We can't drop support for IE, so we're going to have to disable the es6 rules from the google plugin. Adding the following to the "rules" block takes care of it:

"arrow-parens": 0,
"constructor-super": 0,
"generator-star-spacing": 0,
"no-new-symbol": 0,
"no-this-before-super": 0,
"no-var": 0,
"prefer-rest-params": 0,
"prefer-spread": 0,
"rest-spread-spacing": 0,
"yield-star-spacing": 0

@thevoiceofzeke thevoiceofzeke dismissed stale reviews from apetro and vertein March 17, 2017 16:09

Merging as-is would break MyUW in IE

IE11 does not support ES2015 features such as arrow functions, classes,
symbols, block scoped variables, constants, etc.
Google code style rules that recommend these features have been disabled
to allow for IE support.
@ChristianMurphy
Copy link
Contributor Author

Thanks for following up @thevoiceofzeke!
I've updated the PR to remove ES2015+ recommendations from the linter.

@coveralls
Copy link

coveralls commented Mar 17, 2017

Coverage Status

Changes Unknown when pulling 7cd7713 on ChristianMurphy:eslint into ** on UW-Madison-DoIT:master**.

@vertein vertein merged commit cd18b8e into uPortal-Attic:master Mar 17, 2017
@ChristianMurphy ChristianMurphy deleted the eslint branch March 17, 2017 16:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants