Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added import linting #681

Merged
merged 5 commits into from Dec 20, 2016

Conversation

Projects
None yet
5 participants
@shubheksha
Copy link
Contributor

commented Dec 16, 2016

Fixes #442

import parseJSON from 'parse-json';

import defaultSourceWatcher from '../watcher';
import {zipDir} from '../util/zip-dir';
import getValidatedManifest, {getManifestId} from '../util/manifest';
import {prepareArtifactsDir} from '../util/artifacts';
import {createLogger} from '../util/logger';
import {UsageError} from '../errors';
import {UsageError} from '../errors'; //eslint-disable-line import/order

This comment has been minimized.

Copy link
@shubheksha

shubheksha Dec 16, 2016

Author Contributor

@kumar303, I spoke to @rpl about this but both of us couldn't figure out while ESLint was complaining at this point. So resorted to this as nothing else seemed to work.

This comment has been minimized.

Copy link
@shubheksha

shubheksha Dec 16, 2016

Author Contributor

This happens in a couple of other places as well. However, it works in run.js. I've been able to figure out why. I tried mimicking everything as is but still it didn't work.

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 16, 2016

Member

It's because all imports need to be grouped together. It's not the most helpful message but it's telling you that you forgot to group some imports. Here is a patch that fixes it:

diff --git a/src/cmd/build.js b/src/cmd/build.js
index 17a71f9..18c4601 100644
--- a/src/cmd/build.js
+++ b/src/cmd/build.js
@@ -12,17 +12,13 @@ import {zipDir} from '../util/zip-dir';
 import getValidatedManifest, {getManifestId} from '../util/manifest';
 import {prepareArtifactsDir} from '../util/artifacts';
 import {createLogger} from '../util/logger';
-import {UsageError} from '../errors'; //eslint-disable-line import/order
+import {UsageError} from '../errors';
+import type {OnSourceChangeFn} from '../watcher';
+import type {ExtensionManifest} from '../util/manifest';
 
 
 const log = createLogger(__filename);
 
-/* eslint-disable import/order, import/imports-first */
-// Import flow types.
-import type {OnSourceChangeFn} from '../watcher';
-import type {ExtensionManifest} from '../util/manifest';
-/* eslint-disable import/order, import/imports-first */
-
 export function safeFileName(name: string): string {
   return name.toLowerCase().replace(/[^a-z0-9\.-]+/g, '_');
 }

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 16, 2016

Member

you will run into this with all flow type imports. We kept those separate to make it obvious that they are only for flow but I think it's fine to group them with the other parent/sibling imports.

This comment has been minimized.

Copy link
@shubheksha

shubheksha Dec 16, 2016

Author Contributor

Oh, so even though the comment disables the imports for types, it doesn't do it for a normal import and hence ESLint complains? I thought disabling ESLint for the type imports should be enough to deal with this.
However, if we can club module and Flow imports, this won't be an issue at all. I'll go ahead and make changes to all the files in question.

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

@kumar303 @shubheksha 👍 yeah, I agree
in the end, it is fine to group them if it makes the eslint happy and we can use less eslint-disable comments.

@coveralls

This comment has been minimized.

Copy link

commented Dec 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling b0d35a3 on shubheksha:fix/442 into 9643db5 on mozilla:master.

.eslintrc Outdated
// check for bad imports in node_modules.
"node_modules",
// Ignore non-JS imports.
"\\.scss$",

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

@shubheksha we don't have any of these .scss, .jpg, .mp4 or .webm imports in our sources, right?
if it is not currently needed, I would remove the lines between 16 and 20.

.eslintrc Outdated
"flowtype/space-after-type-colon": [1, "always"],

// This makes sure imported modules exist.
"import/no-unresolved": ["error"],

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

@shubheksha in the other rules configured in this .eslintrc file, we currently use 2 in place of "error", we should probably keep it consistent with this new group of rules as well.

(Also, "rule": ["error"] can be probably be rewritten to "rule": 2 for the same reason)

@@ -9,9 +9,9 @@ module.exports = function(grunt) {


// This loads all grunt tasks matching the grunt-*, @*/grunt-* patterns.
require('load-grunt-tasks')(grunt);
require('load-grunt-tasks')(grunt); //eslint-disable-line import/no-extraneous-dependencies

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

I would probably prefer to put all the development related files that are not in the tests dir into the .eslintrc file as file glob patterns, instead of ignoring the issues line by line, e.g.:

`
...
"import/no-extraneous-dependencies": [2, {
   "devDependencies": ["Gruntfile.js", "webpack.*.js", "tasks/*.js"]
}],
...

but let's see what @kumar303 thinks about it before applying this change.

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 16, 2016

Member

yeah, that sounds better to me as well

@@ -13,7 +14,7 @@ import {isErrorWithCode, UsageError, WebExtError} from '../errors';
import {getPrefs as defaultPrefGetter} from './preferences';
import {getManifestId} from '../util/manifest';
import {createLogger} from '../util/logger';
import {default as defaultFirefoxConnector, REMOTE_PORT} from './remote';
import {default as defaultFirefoxConnector, REMOTE_PORT} from './remote';//eslint-disable-line import/order

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

@shubheksha move this line so that this group is ordered is not enough?

if it is not, we should fix the spacing near the eslint-disable-line comment:

.. from './remote'; // eslint-disable-line import/order
import type {ExtensionManifest} from '../util/manifest';

import type {ExtensionManifest} from '../util/manifest'; //eslint-disable-line
/* eslint-enable import/order */

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

the // eslint-disable-line comment is probably a leftover, right?

@@ -16,11 +16,13 @@ export const REMOTE_PORT = 6005;


// RemoteFirefox types and implementation

/* eslint-disable import/order, import/imports-first, import/no-extraneous-dependencies */
//This is related to Flow and will be stripped in production hence won't be treated as a dependency at all

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

nit, fix the spacing in this comment (// This ...)

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

when we have regular comments in the same place where we are going to use an eslint-disable block comment, it would be probably better to move that comment outside the eslint-disable, e.g.:

// This is related to ...
/* eslint-disable ... */
...
/* eslint-enable ... */

and

// import Flow Types
/* eslint-disable ... */
...
/* eslint-enable ... */
import type FirefoxClient from 'firefox-client';

export type FirefoxConnectorFn =
(port?: number) => Promise<FirefoxClient>;
/* eslint-disable import/order, import/imports-first */

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

@shubheksha This seems that it should be an eslint-enable comment

Also, I'm wondering if export type FirefoxConnectorFn = ... needs to be inside the eslint-disable comment.

import {RemoteFirefox} from '../../../src/firefox/remote';

const defaultFirefoxEnv = firefox.defaultFirefoxEnv;

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

This can be probably merged with line 11 and use the syntax import firefox, {defaultFirefoxEnv} from '...';

This comment has been minimized.

Copy link
@shubheksha

shubheksha Dec 16, 2016

Author Contributor

I tried that, but it looks like there are no default imports in firefox.js hence it fails.

This comment has been minimized.

Copy link
@rpl

rpl Dec 16, 2016

Member

@shubheksha yeah, you are right, it looks like we have no nice ES6 syntax to merge the two (all named exports, '*', into the 'firefox' alias and the single 'defaultFirefoxEnv' named export with a single import statement :-( )

That's not a big deal, how about just changing the above line to:

const {defaultFirefoxEnv} = firefox;
@rpl

This comment has been minimized.

Copy link
Member

commented Dec 16, 2016

Thanks @shubheksha! this "imports linting" is going to help a lot!

It is almost done, I added some comments, you can already handle/discuss with me the most important ones and then @kumar303 is going to take a second look.

@coveralls

This comment has been minimized.

Copy link

commented Dec 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 12a80d2 on shubheksha:fix/442 into 9643db5 on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

commented Dec 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 0e262af on shubheksha:fix/442 into 9643db5 on mozilla:master.

@kumar303
Copy link
Member

left a comment

Great, thanks for slogging through all this. The missing import checks will be especially useful. I requested some minor cleanup.

.eslintrc Outdated
"import/ignore": [
// Because of CommonJS incompatibility, we can't
// check for bad imports in node_modules.
"node_modules",

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 19, 2016

Member

Huh, I forget what the exact issue was here in addons-frontend. I just removed it and web-ext has no complaints :) Let's remove this entire import/ignore block from the config for now until we run into specific problems.

.eslintrc Outdated
"import/resolver": {
"node": {
// This adds ./src for relative imports.
"moduleDirectory": ["node_modules", "src"]

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 19, 2016

Member

Since our relative imports all use exact paths (unlike addons-frontend), adding src here isn't necessary. Also, since node_modules is a default, you can just remove this entire import/resolver block.

@@ -5,5 +5,9 @@
},

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 19, 2016

Member

Huh, I just noticed mocha: true in env. You can remove that. That rule allowed us to reference describe() as a global but we never do that (we import it) so the rule is not necessary.

@@ -5,5 +5,9 @@
},
"rules": {
"strict": [2, "never"],

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 19, 2016

Member

we already had strict in the main .eslintrc so you can remove it from here

import type {
FirefoxProcess,
FirefoxProcess, //eslint-disable-line import/named

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 19, 2016

Member

aww, it doesn't work with the flow linter? That's too bad. This is a good way to workaround it though.

This comment has been minimized.

Copy link
@shubheksha

shubheksha Dec 20, 2016

Author Contributor

I think FirefoxProcess in particular doesn't work because it's declared as an interface in index.js


// Import flow types.

//Import flow types

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 19, 2016

Member

nit: we generally put a space after the slashes for readable comments like // Import flow types. We don't have a lint rule for that because when you comment out some code temporarily you get yelled at which can be annoying.

@@ -15,7 +16,7 @@ import completeSignCommand, {
extensionIdFile, getIdFromSourceDir, saveIdToSourceDir,
} from '../../../src/cmd/sign';
import {makeSureItFails, fixturePath} from '../helpers';

//Import flow type

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 19, 2016

Member

nit: again, space after the slashes

@shubheksha shubheksha force-pushed the shubheksha:fix/442 branch from 0e262af to bf67a48 Dec 20, 2016

@coveralls

This comment has been minimized.

Copy link

commented Dec 20, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling bf67a48 on shubheksha:fix/442 into 0b62637 on mozilla:master.

@kumar303
Copy link
Member

left a comment

Thanks, about to merge! Let's see how many conflicts it creates in other branches.
aziz-ansari

@kumar303 kumar303 merged commit 5e8b288 into mozilla:master Dec 20, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.