chore(eslint): Configure ESLint Fixes #9261 #10824

graingert · 2017-07-31T18:07:50Z

No description provided.

graingert · 2017-07-31T18:08:32Z

~~hmm travis isn't even running~~ oh yes it is

graingert · 2017-07-31T18:09:03Z

.eslintrc.json

+        "wrap-regex": "off",
+        "yield-star-spacing": "error",
+        "yoda": "off",
+        "no-unused-vars": "off",


I had to turn this off because there's loads of these!

graingert · 2017-07-31T18:09:42Z

.eslintrc.json

+                "jasmine": true
+            },
+            "rules": {
+                "no-native-reassign": "off",


what are you guys doing 🤕

graingert · 2017-07-31T18:11:00Z

src/components/tabs/tabsPaginationService.spec.js

@@ -1,6 +1,6 @@
 describe('MdTabsPaginationService', function() {

-  const TAB_WIDTH = 100;


nice try at ES2015 support.

You should configure babel properly and use let/const for everything if you're going to use them

@mgol re: the es6 flag es2015 constructs are banned ^

graingert · 2017-07-31T18:11:16Z

src/core/services/layout/layout.js

        case 'layout-margin'  :
        case 'layout-fill'    :
        case 'layout-wrap'    :
-        case 'layout-nowrap'  :


graingert · 2017-07-31T18:12:59Z

src/core/services/theming/theming.js

      self[colorType + 'Color'] = function() {
        var args = Array.prototype.slice.call(arguments);
+        // eslint-disable-next-line no-console
        console.warn('$mdThemingProviderTheme.' + colorType + 'Color() has been deprecated. ' +


should use probably use:

$injector.invoke(['$log', function($log) { $log.warn(...); });

mgol · 2017-08-16T11:36:51Z

src/core/services/theming/theming.js


      self[colorType + 'Color'] = function() {
        var args = Array.prototype.slice.call(arguments);
+        // eslint-disable-next-line no-console


Do we really want no-console in tests?

This doesn't look like a test. We always want no-console because Angular provides a log service

mgol · 2017-08-16T11:38:08Z

src/components/navBar/demoBasicUsage/script.js

    $scope.currentNavItem = 'page1';

    $scope.goto = function(page) {
-      console.log("Goto " + page);


I'd leave console.logs as they are outside of source.

It's bad form to recommend use of globals that Angular provides injectables for. It leads to the spreading of bad habits

mgol · 2017-08-16T11:39:08Z

.eslintrc.json

@@ -0,0 +1,271 @@
+{
+    "env": {
+        "es6": true


Why enable es6? I think ES5 should be enforced which also means parserOptions.ecmaVersion needs to be set to 5.

Oh pretty sure this flag doesn't do anything anymore

How so? It defines ES2015 globals like Set or Promise. Wed don't want that here as some of our supported environments don't have those globals.

well I'm going to remove it anyway

mgol · 2017-08-16T11:39:53Z

I added some comments. At a very first sight this looks good but someone from the Material team should verify, especially with regards to concrete rules defined. cc @ThomasBurleson @EladBezalel

graingert · 2017-08-16T11:46:10Z

@mgol I auto-generated the rules, it's based on what rules from eslint recommended already pass

graingert · 2017-08-16T11:49:23Z

@mgol thanks for looking at this PR, I'd rather you look at #10826 because that makes some changes to comply with a stricter set of eslint rules. (And I'm running it in production now)

graingert · 2017-08-16T11:54:09Z

@mgol

especially with regards to concrete rules defined

angular-material could definitely do with switching to a stricter styleguide, it's got a lot of inconsistent and redundant stuff in.

mgol · 2017-08-16T12:48:54Z

I'm confused, #10826 seems to have lots of unrelated commits. Single PRs should be about a single thing as otherwise it's hard to review. There should be one PR for ESLint setup, then another for any $window-related changes etc.

mgol · 2017-08-16T12:49:24Z

.eslintrc.json

@@ -0,0 +1,268 @@
+{
+    "extends": "eslint:recommended",


I'd rather not extend any built-in preset as they can change. We should define our own rules ourselves.

@mgol eslint:recommended is a very carefully managed preset, and any changes to it are signaled via semver.

graingert · 2017-08-23T10:08:51Z

@ThomasBurleson can I get a review here or #10826 please?

graingert · 2017-09-25T23:36:45Z

@ThomasBurleson rebased over conflicts

Splaktar · 2017-11-18T06:37:50Z

src/components/dialog/dialog.js

      options.hideBackdrop = function hideBackdrop($destroy) {
        if (options.backdrop) {
-          if ( !!$destroy ) options.backdrop.remove();
+          if ( $destroy ) options.backdrop.remove();


ESLint says that !! should be replaced by Boolean($destroy) for this rule. Is there a reason that you don't think that guidance applies here and in the other instances of this replacement in the PR?

This was autofixed. It's because there's no need to coerce with if ( ... )

also no-implicit-coercion is not enabled in eslint:recommended https://eslint.org/docs/rules/

This is the rule that applied in this location: https://eslint.org/docs/rules/no-extra-boolean-cast

Splaktar · 2017-11-18T06:39:23Z

Is there an associated issue or forum thread where adding ESLint to the project was discussed?

graingert · 2017-11-20T10:28:41Z

@Splaktar #9261

Splaktar

Looks like a good start to me.

graingert · 2017-11-21T10:13:29Z

@Splaktar there's stuff in the merge queue from back in March when are you expecting to actually merge them?

Splaktar · 2017-11-22T08:32:37Z

@graingert PRs with the merge ready label are getting run through presubmit tonight. Some of them may be merged tomorrow. Our top priority atm is to catch up with the merge queue and start putting out small releases on a regular basis.

andrewseguin · 2017-12-12T00:19:26Z

Needs rebase

graingert · 2017-12-12T00:33:39Z

@andrewseguin done

graingert · 2017-12-12T00:36:38Z

@Splaktar how did presubmit go?

andrewseguin · 2017-12-12T17:57:13Z

Please rebase again

graingert · 2017-12-12T17:58:46Z

Please rebase again

😡

graingert · 2017-12-12T18:11:37Z

@andrewseguin rebased. Can you merge this one in before the others, otherwise I have to fix everyone else's eslint violations each time.

Splaktar · 2017-12-13T07:21:54Z

@graingert sorry for the delay and the ordering of the merges, doh. Glad that we got this in though. Thank you!

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jul 31, 2017

graingert commented Jul 31, 2017

View reviewed changes

.eslintrc.json

"jasmine": true

},

"rules": {

"no-native-reassign": "off",

Copy link

Contributor Author

graingert Jul 31, 2017

Choose a reason for hiding this comment

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

what are you guys doing 🤕

graingert commented Jul 31, 2017

View reviewed changes

graingert force-pushed the eslint branch 2 times, most recently from 7648e85 to 1988972 Compare August 1, 2017 09:55

graingert changed the title ~~Configure ESLint Fixes #9261~~ chore(eslit): Configure ESLint Fixes #9261 Aug 1, 2017

graingert changed the title ~~chore(eslit): Configure ESLint Fixes #9261~~ chore(eslint): Configure ESLint Fixes #9261 Aug 1, 2017

graingert mentioned this pull request Aug 1, 2017

fix(all): remove all direct use of browser globals #10826

Closed

mgol reviewed Aug 16, 2017

View reviewed changes

graingert force-pushed the eslint branch from 1988972 to 468d271 Compare August 16, 2017 11:52

mgol reviewed Aug 16, 2017

View reviewed changes

ThomasBurleson modified the milestone: 1.1.6 Sep 5, 2017

graingert force-pushed the eslint branch from 468d271 to 3bc5553 Compare September 5, 2017 09:53

graingert mentioned this pull request Sep 5, 2017

No browser globals 2 #10886

Closed

graingert force-pushed the eslint branch from 3bc5553 to 00a4312 Compare September 25, 2017 23:36

Splaktar reviewed Nov 18, 2017

View reviewed changes

graingert force-pushed the eslint branch from 00a4312 to a86d127 Compare November 20, 2017 14:01

Splaktar approved these changes Nov 21, 2017

View reviewed changes

Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Nov 21, 2017

Splaktar added P4: minor Minor issues. May not be fixed without community contributions. type: chore labels Dec 11, 2017

andrewseguin added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Dec 12, 2017

graingert force-pushed the eslint branch from fd1b86a to 97ee1ec Compare December 12, 2017 00:32

Thomas Grainger and others added 5 commits December 12, 2017 18:06

chore(eslint): add eslint with some small fixes

327400e

chore(eslint): fix no-useless escape

a1e3888

chore(eslint): fix no-extra-boolean-cast

213e523

build(ci): enable eslint on travis Fixes angular#9261

4f31e17

configure eslint to be ES2017

4df9e48

graingert force-pushed the eslint branch from 97ee1ec to 4df9e48 Compare December 12, 2017 18:10

andrewseguin merged commit d9eb909 into angular:master Dec 12, 2017

graingert deleted the eslint branch December 12, 2017 22:57

Splaktar mentioned this pull request Jan 12, 2018

build: configure ESLint coverage for gulp, scripts, docs, config #11064

Merged

3 tasks

		@@ -1,6 +1,6 @@
		describe('MdTabsPaginationService', function() {

		const TAB_WIDTH = 100;

chore(eslint): Configure ESLint Fixes #9261 #10824

chore(eslint): Configure ESLint Fixes #9261 #10824

Uh oh!

Conversation

graingert commented Jul 31, 2017

Uh oh!

graingert commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graingert Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graingert Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol commented Aug 16, 2017

Uh oh!

graingert commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graingert commented Aug 16, 2017

Uh oh!

graingert commented Aug 16, 2017

Uh oh!

mgol commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graingert Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graingert commented Aug 23, 2017

Uh oh!

graingert commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Nov 18, 2017

Uh oh!

graingert commented Nov 20, 2017

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

graingert commented Nov 21, 2017

Uh oh!

Splaktar commented Nov 22, 2017

graingert commented Jul 31, 2017 •

edited

Loading

graingert Jul 31, 2017 •

edited

Loading

graingert Jul 31, 2017 •

edited

Loading

graingert commented Aug 16, 2017 •

edited

Loading

graingert Aug 16, 2017 •

edited

Loading