Check module import validity with eslint-plugin-import #12303

jsnmoon · 2017-03-19T23:45:44Z

(Assists #11688.)

This change adds eslint-plugin-import to our ESLint configuration. It uses either a Webpack resolver or a Node resolver depending on the path of the linted file.

This change helps us verify the validity of existing imports as well as ensuring correctness for any new imports we introduce into the project. This will be especially important as we begin migrating the client module notation from CommonJS to ES.

Todo List:

Correctly parse imports inside the /client/ directory
Correctly parse imports inside the /server/ directory
Correctly parse imports inside a /test/ directory

CC @ockham @aduth @ehg: Your input would be greatly appreciated!

oandregal · 2017-03-23T09:38:48Z

Howdy Jason, regarding the changes in .eslintrc: with the merge of #6945 you may set any new rules as errors. All new code will be reported as an error if it doesn't honor the rule, but the legacy code will be reported as warning, so our CI/hook tools won't complain about it.

jsnmoon · 2017-03-25T07:44:19Z

Hey Andrés, thanks for the heads up! That's good to know :)

aduth · 2017-03-29T18:38:45Z

webpack.config.js

@@ -64,15 +65,7 @@ const webpackConfig = {
 			}
 		]
 	},
-	resolve: {


Does this need to be moved to a separate config? Wouldn't it be enough to resolve using the webpack.config.js file from .eslint-resolver.js as-is?

Hi @aduth, the main reason I had separated the resolve into its own separate file was due to the fact that the dependencies for webpack.config.js assumes a different root than the node paths ESLint is instantiated with. For example, webpack.config.js has lib/create-config, which is a dependency in the client folder; ESLint can't easily resolve this. (An easy way to repro this: node -e "var config = require('./webpack.config');")

I don't know of an easy way to modify the node paths for ESLint, which is why I decided to separate the resolve configs to a new file.

aduth · 2017-03-29T18:39:30Z

.eslint-resolver.js

+
+exports.interfaceVersion = 2
+
+exports.resolve = function (source, file, config) {


Ironically your ESLint resolver script has many lint errors 😄 (spacing after function, between parens).

I realized that ESLint was ignoring the file due to the .eslint prefix. I went ahead and changed .eslintignore to force parsing .eslint-resolver.js so this shouldn't happen again.

aduth · 2017-03-29T18:41:43Z

.eslint-resolver.js

+ */
+const log = debugFactory('eslint-plugin-import:resolver:wp-calypso-resolver');
+
+const nodeResolverConfig = {


Can you explain the need for separate resolving configs? Should this be pulled from webpack.config.node.js for server-side module importing?

Sure - the reason why we need separate resolving configs is due to how files across our project assume a different root directory. For client files, this includes /client/
and /client/extensions/; for server files, this includes /, /client/, and /server/. Test files tend to add /test/ as a root directory as well.

I do think it should be pulling from webpack.config.node.js for server-side module importing. I'll push out a revision to address this shortly.

aduth · 2017-03-29T18:43:30Z

.eslintrc.js

+/* eslint-disable import/no-commonjs */
+
+const path = require( 'path' );
+const customResolverPath = path.join( __dirname, './.eslint-resolver.js' );


Won't the result of this join be simply "./.eslint-resolver.js" which we could instead define as a string key of the settings, not a computed property?

Yes, you're absolutely correct. I'll fix this.

EDIT: I initially thought this was the case, but I overlooked the fact that the result of path.join is the absolute path of the resolver file (e.g. /Users/myUsername/wp-calypso/.eslint-resolver.js). Unless an absolute path is specified as the Object key, CircleCI fails to run run-lint properly.

aduth · 2017-03-29T18:45:09Z

.eslintrc.js

 	rules: {
 		camelcase: 0, // REST API objects include underscores
+		// NOTE: Some import rules are errors in client (Webpack's resolution) and
+		//       warnings in server (Node's module resolution).


Can't we still define these all as errors and expect the server-specific configuration to override them?

Yes, we can define all of these rules as errors and override them inside server/.eslintrc.json. The following rules are already overridden for the server:

Set to warn:

import/default

import/export

import/named

import/namespace

Set to ignore:
- import/no-commonjs

Which rules would you recommend upgrading to error in .eslintrc.js? The following rules are currently set as warnings for the client:

import/no-commonjs

import/no-named-as-default-member

import/no-named-as-default

import/no-webpack-loader-syntax

aduth · 2017-03-29T18:46:09Z

index.js

@@ -1,4 +1,5 @@
 /* eslint-disable no-console */
+/* eslint-disable import/no-commonjs */


FYI you can comma-separate disabled rules in a single eslint-disable comment.

Got it 👍

This custom resolver inspects the file's path to decide which off-the-shelf resolver would be appropriate. For files from the client folder, this resolver uses Webpack's resolver. For other files, this resolver uses Node's resolver.

Peak irony has been reached.

This reverts commit 127a900.

matticbot · 2017-04-12T15:08:08Z

@jsnmoon This PR needs a rebase

jsnmoon · 2017-07-25T20:39:18Z

Given that we're beginning to rely more in Prettier, I think we can put this PR on hold indefinitely for now. I'll leave the branch alive for anyone who'd like to use this as a reference to integrate import path resolution with ESLint.

jsnmoon added [Status] In Progress [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Mar 19, 2017

matticbot added the OSS Citizen label Mar 19, 2017

jsnmoon force-pushed the try/eslint-plugin-import branch 2 times, most recently from de34610 to 90f6502 Compare March 20, 2017 00:15

designsimply added the Build label Mar 21, 2017

jsnmoon force-pushed the try/eslint-plugin-import branch from 90f6502 to e85d027 Compare March 22, 2017 18:36

jsnmoon force-pushed the try/eslint-plugin-import branch from 7c88a27 to df4944e Compare March 25, 2017 07:48

matticbot added the [Size] M Medium sized issue label Mar 26, 2017

jsnmoon force-pushed the try/eslint-plugin-import branch 2 times, most recently from c8bc0f0 to cb47d9f Compare March 27, 2017 22:10

jsnmoon added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 27, 2017

jsnmoon self-assigned this Mar 28, 2017

matticbot added the [Size] M Medium sized issue label Mar 28, 2017

aduth reviewed Mar 29, 2017

View reviewed changes

matticbot added [Size] L Large sized issue and removed [Size] M Medium sized issue labels Apr 4, 2017

jsnmoon added 10 commits April 3, 2017 20:47

Webpack: refactor resolve config to separate file

0d819e2

npm: Add eslint-plugin-import and resolvers

78f90cf

ESLint: Add import lint rules for client & server

1aa8ba1

ESLint: Disable CommonJS rule in non-Webpack files

14b2f38

ESLint: Use custom module resolver for ESLint

71e4412

This custom resolver inspects the file's path to decide which off-the-shelf resolver would be appropriate. For files from the client folder, this resolver uses Webpack's resolver. For other files, this resolver uses Node's resolver.

ESLint: Fix all linting errors inside resolver

b50f16c

Peak irony has been reached.

ESLint: Use string for custom resolver path

127a900

ESLint: Merge rule overrides into one line

9e48969

ESLint: Parse eslint resolver script with eslint

00c455c

ESLint: Add exception to no-commonjs rule

8c91101

jsnmoon added 2 commits April 3, 2017 21:16

ESLint: Merge rule overrides into one line, take 2

74d876c

ESLint: Split out resolve config from node webpack

5fa8734

jsnmoon force-pushed the try/eslint-plugin-import branch from a236882 to 5fa8734 Compare April 4, 2017 04:37

ESLint: Update resolver to use Webpack for server

ac00382

jsnmoon requested review from ockham and ehg April 4, 2017 19:51

jsnmoon added 3 commits April 4, 2017 12:56

ESLint: Use string for custom resolver path take 2

9f55173

ESLint: Remove unnecessary imports for resolver

285c5af

ESLint: Clarify comment in .eslintrc.js

da733cb

jsnmoon requested a review from oandregal April 4, 2017 20:35

Revert "ESLint: Use string for custom resolver path"

b9a7d5d

This reverts commit 127a900.

matticbot added the [Status] Needs Rebase label Apr 12, 2017

jsnmoon mentioned this pull request Apr 18, 2017

Migrate CommonJS to ES2015 module notation (imports and exports) #11688

Closed

11 tasks

jsnmoon closed this Jul 25, 2017

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 25, 2017

alisterscott deleted the try/eslint-plugin-import branch October 16, 2017 03:37

dmsnell mentioned this pull request Jan 23, 2018

Framework/no implicit import paths #20142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check module import validity with eslint-plugin-import #12303

Check module import validity with eslint-plugin-import #12303

jsnmoon commented Mar 19, 2017 •

edited

Loading

oandregal commented Mar 23, 2017

jsnmoon commented Mar 25, 2017

aduth Mar 29, 2017

jsnmoon Apr 4, 2017 •

edited

Loading

aduth Mar 29, 2017

jsnmoon Mar 30, 2017

jsnmoon Apr 4, 2017 •

edited

Loading

aduth Mar 29, 2017

jsnmoon Apr 4, 2017

aduth Mar 29, 2017

jsnmoon Mar 30, 2017 •

edited

Loading

aduth Mar 29, 2017

jsnmoon Apr 4, 2017

aduth Mar 29, 2017

jsnmoon Mar 30, 2017

matticbot commented Apr 12, 2017

jsnmoon commented Jul 25, 2017


		exports.interfaceVersion = 2

		exports.resolve = function (source, file, config) {

		@@ -1,4 +1,5 @@
		/* eslint-disable no-console */
		/* eslint-disable import/no-commonjs */

Check module import validity with eslint-plugin-import #12303

Check module import validity with eslint-plugin-import #12303

Conversation

jsnmoon commented Mar 19, 2017 • edited Loading

oandregal commented Mar 23, 2017

jsnmoon commented Mar 25, 2017

Choose a reason for hiding this comment

jsnmoon Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnmoon Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnmoon Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matticbot commented Apr 12, 2017

jsnmoon commented Jul 25, 2017

jsnmoon commented Mar 19, 2017 •

edited

Loading

jsnmoon Apr 4, 2017 •

edited

Loading

jsnmoon Apr 4, 2017 •

edited

Loading

jsnmoon Mar 30, 2017 •

edited

Loading