Build: ESLint setup improvements #3385

mgol · 2016-11-02T23:52:45Z

Summary

Checklist

Mark an [x] for completed items, if you're not sure leave them unchecked and we can assist.

All authors have signed the CLA at https://contribute.jquery.com/CLA/
~~New tests have been added to show the fix or feature works~~
Grunt build and unit tests pass locally with these changes
~~If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com~~

Thanks! Bots and humans will be around shortly to check it out.

Use the short name of the preset in the config.
Run ESLint first on non-minified files.
Explicitly specify environments in every config file (those settings cascade
which means we've been assuming a Node.js environment where we shouldn't have).

I found these when working on jquery/jquery-migrate#238.

cc @markelog

mention-bot · 2016-11-02T23:52:46Z

@mgol, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @timmywil and @mikesherov to be potential reviewers.

markelog · 2016-11-03T00:15:51Z

You might as well rephrase

jquery/Gruntfile.js

Line 113 in 5b4cb0d

// See https://github.com/sindresorhus/grunt-eslint/issues/119

it seems @sindresorhus didn't restore those issues – https://twitter.com/arkel/status/760518498699411457

Also might be a good idea to remove this too

mgol · 2016-11-03T00:35:38Z

Yeah. Maybe a little description could be useful since you can't view the ticket.

mgol · 2016-11-03T00:35:54Z

And good catch about the JSCS mention!

sindresorhus · 2016-11-03T02:55:00Z

You might as well rephrase

jquery/Gruntfile.js

Line 113 in 5b4cb0d

// See https://github.com/sindresorhus/grunt-eslint/issues/119

it seems @sindresorhus didn't restore those issues – https://twitter.com/arkel/status/760518498699411457

I forgot. Done now.

markelog · 2016-11-03T02:59:18Z

@sindresorhus cool, thank you

mgol · 2016-11-07T17:21:21Z

@markelog I deleted the line but it seems to not exactly fit into the first commit, I added it as a separate one. What do you think?

markelog · 2016-11-03T00:12:35Z

Gruntfile.js

 	] );

 	grunt.registerTask( "lint:newer", [
 		"newer:jsonlint",
-		runIfNewNode( "newer:eslint" )


Same idea as in https://github.com/jquery/jquery-migrate/pull/238/files#diff-35b4a816e0441e6a375cd925af50752cR129 I suppose?

I wouldn't be oppose to comment about this in the code though

Yeah, commenting that makes sense, otherwise someone will remove it eventually. ;)

markelog · 2016-11-03T00:13:47Z

src/.eslintrc.json

@@ -1,4 +1,8 @@
 {
+	"env": {
+		"browser": false,


To be more explicit? Why in these files only?

What do you mean by "these files only"?

The configs cascade down in ESLint. If you don't specify browser: false, you depend on the parent config to not specify browser: true. Since we have to reset the node env, I wanted to specify the browser one explicitly, it seems less fragile.

Yeah, that's what I mean, you making only these configs explicit while others are still "less fragile", i'm wondering why?

Ah, I get it now. I'll update the others as well. I'd only leave the ones that explicitly extend src/.eslintrc.json as there we do want to replicate the same settings for source files.

markelog · 2016-11-09T12:24:09Z

@markelog I deleted the line but it seems to not exactly fit into the first commit, I added it as a separate one. What do you think?

I find you approach marvellous :)

mgol · 2016-11-16T11:21:16Z

@markelog PR updated, PTAL.

markelog · 2016-11-16T20:17:13Z

test/.eslintrc.json

@@ -1,4 +1,6 @@
 {
+	"extends": "../src/.eslintrc.json",
+	"root": true,


mmm, so this is root, so it wouldn't inherit upper configs, only extends one from src and src one doesn't have root so it would inherit upper ones, which would be the same uppers since src is sibling of test dir, correct?

Yeah, you're right. :D How about we create .eslintrc-(base|browser|node).json files and extend whichever we need? Similarly to what I did for Angular; see https://github.com/angular/angular.js.

Hm, how about we extend only eslint-jquery-config with root: true in every config? It will cause some repetition, but it could make it very clear. What do you think?

We'd have to duplicate the globals in every src-related .eslintrc.json. What's the drawback of what I proposed?

Yeah, I guess duplicating of globals would suck more.

What's the drawback of what I proposed?

Angular historically didn't shy away from those in their repo so it's seems fine for them, but for me personally that seems as a clutter

Didn't shy from what exactly?

Is my proposal of the .eslintrc-(base|browser|node).json files OK for you?

From dot files

Is my proposal of the .eslintrc-(base|browser|node).json files OK for you?

If that's the best option we can come up with...

mgol · 2016-11-30T14:33:08Z

@markelog PR updated, PTAL.

markelog · 2016-12-05T13:06:05Z

.eslintrc.json

-	"env": {
-		"node": true
-	}
+	"extends": "./.eslintrc-node.json"


What about root: true?

markelog · 2016-12-05T13:08:39Z

test/.eslintrc.json

+	"root": true,
+
+	"extends": "../.eslintrc-browser.json",
+
 	"env": {
 		"browser": true


Should this be in .eslintrc-browser.json only?

I'm not sure how to best play it out. The issue is that we don't want browser: true to be specified in source as we take all the browser stuff from the window variable and that's important for jsdom compatibility. On the other hand, it would be too much to fix in tests and that would only help us if we ever tried to run the test suite (or its parts) in jsdom which we don't have plans currently. So setup between src & test will differ.

What would you suggest?

I'm thinking set it in .eslintrc-browser.json and disable it in src seems more logical that way?

@markelog I was thinking that generally our browser setup should be made work with all browser globals put on window and not necessarily available a globals and that tests are the exception, not source.

(Although it seems .eslintrc-browser.json should have a comment explaining that)

So we always want to reference global vars from window object? Yeah I recall we talked about this before, sounds good to me then – a default disabled browser option and additional comment

So we always want to reference global vars from window object?

I'm sure you meant it :) but just to be precise - only the browser-only globals; the one from the ECMAScript specification are used directly.

I'll update the PR.

@markelog PR updated

1. Use the short name of the preset in the config. 2. Run ESLint first on non-minified files. 3. Explicitly specify environments in every config file (those settings cascade which means we've been assuming a Node.js environment where we shouldn't have).

The file doesn't exist anymore.

This way `eslint .` run from the terminal will work regardless of ignored files present in the repository. Closes jquerygh-3385

Fixes Node smoke tests Ref jquerygh-3385

Fixes Node smoke tests Ref gh-3385 Closes gh-3460

mgol added the Build label Nov 2, 2016

mgol self-assigned this Nov 2, 2016

jquerybot added the CLA: Valid label Nov 2, 2016

JSFOwner removed the CLA: Valid label Nov 5, 2016

markelog reviewed Nov 8, 2016

View reviewed changes

markelog reviewed Nov 16, 2016

View reviewed changes

mgol force-pushed the eslint branch from 79673d5 to b1d65af Compare November 30, 2016 14:32

markelog reviewed Dec 5, 2016

View reviewed changes

timmywil modified the milestone: 3.2.0 Dec 12, 2016

mgol force-pushed the eslint branch from f671b11 to 0638c09 Compare December 13, 2016 09:11

markelog approved these changes Dec 16, 2016

View reviewed changes

mgol added 3 commits December 19, 2016 02:07

Build: ESLint setup improvements

cbc8638

1. Use the short name of the preset in the config. 2. Run ESLint first on non-minified files. 3. Explicitly specify environments in every config file (those settings cascade which means we've been assuming a Node.js environment where we shouldn't have).

Build: Drop jscs.json from .npmignore

b3e0a56

The file doesn't exist anymore.

Build: Add most of the dist folder to .eslintignore

1754e31

This way `eslint .` run from the terminal will work regardless of ignored files present in the repository. Closes jquerygh-3385

mgol force-pushed the eslint branch from 0638c09 to 1754e31 Compare December 19, 2016 01:08

mgol merged commit 1754e31 into jquery:master Dec 19, 2016

mgol deleted the eslint branch December 19, 2016 01:16

gibson042 added a commit to gibson042/jquery that referenced this pull request Dec 19, 2016

Build: Use valid ecmaVersion

b008305

Fixes Node smoke tests Ref jquerygh-3385

gibson042 mentioned this pull request Dec 19, 2016

Build: Use valid ecmaVersion #3460

Merged

gibson042 added a commit that referenced this pull request Dec 19, 2016

Build: Use valid ecmaVersion

8cb4cd7

Fixes Node smoke tests Ref gh-3385 Closes gh-3460

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Build: ESLint setup improvements #3385

Build: ESLint setup improvements #3385

Uh oh!

Conversation

mgol commented Nov 2, 2016

Summary

Checklist

Uh oh!

mention-bot commented Nov 2, 2016

Uh oh!

markelog commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgol commented Nov 3, 2016

Uh oh!

mgol commented Nov 3, 2016

Uh oh!

sindresorhus commented Nov 3, 2016

Uh oh!

markelog commented Nov 3, 2016

Uh oh!

mgol commented Nov 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog Nov 14, 2016 • 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!

markelog commented Nov 9, 2016

Uh oh!

mgol commented Nov 16, 2016

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 Nov 30, 2016

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markelog commented Nov 3, 2016 •

edited

Loading

markelog Nov 14, 2016 •

edited

Loading