Add `import-style` rule #232

rahgurung · 2019-02-02T05:42:05Z

Fixed #211

Things Done:

Throws error on directly importing default modules like utils, lodash, etc with support for overriding the objects to add own modules.

sindresorhus · 2019-02-02T12:07:34Z

(I just need a little help on the todo part, can you give me a code pointer or any current rule which does this, I will come up with a solution.)

This rule accepts options: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/rules/catch-error-name.js See the schema part. More here; https://eslint.org/docs/developer-guide/working-with-rules#options-schemas

test/no-direct-import.js

docs/rules/no-direct-import.md

readme.md

index.js

readme.md

sindresorhus · 2019-02-02T12:16:41Z

The rule should support enforcing destructuring and using default import, as commented in #211 (comment) So I think the rule needs a better name. It can't have the no- prefix:

we could also have a list for the inverse? For example, chalk should not be destructured, but people still do. Same with the core path module.

rahgurung · 2019-02-05T15:44:13Z

The rule should support enforcing destructuring and using default import, as commented in #211 (comment) So I think the rule needs a better name. It can't have the no- prefix:

we could also have a list for the inverse? For example, chalk should not be destructured, but people still do. Same with the core path module.

how about prefer-default-import ? @sindresorhus

futpib · 2019-02-05T18:19:12Z

How about import-specifier-style or import-destructuring?

rahgurung · 2019-02-06T09:28:21Z

@sindresorhus

sindresorhus · 2019-02-06T15:57:44Z

How about import-specifier-style or import-destructuring?

The specifier is the right side of the import statement. The braces in import is also not destructuring.

How about just import-style or import-default-or-named?

rahgurung · 2019-02-06T16:56:21Z

Another problem is how can I work for the inverse part, can I take an input from user where he will input something like import-style: inverse and those module names will be linted in inverse way ?

rahgurung · 2019-02-06T16:57:28Z

How about import-specifier-style or import-destructuring?

The specifier is the right side of the import statement. The braces in import is also not destructuring.

How about just import-style or import-default-or-named?

Import-style sounds good to me,

futpib · 2019-02-07T16:40:03Z

The specifier is the right side of the import statement. The braces in import is also not destructuring.

Specifier is the left-hand side. In case of a require call it can instead be a destructring. import-style is ok

sindresorhus · 2019-02-07T16:48:55Z

Specifier is the left-hand side.

I just Google'd "javascript import specifier" and all the sources claim it's the right part, like: https://nodejs.org/api/esm.html#esm_url_based_paths

sindresorhus · 2019-02-07T16:50:07Z

Alright, let's go with import-style.

sindresorhus · 2019-02-07T16:55:56Z

Another problem is how can I work for the inverse part, can I take an input from user where he will input something like import-style: inverse and those module names will be linted in inverse way ?

The rule should accepts an options-object like this:

{
	defaultExport: [
		'path',
		'chalk'
	],
	namedExport: [
		'util',
		'lodash',
		'underscore'
	]
}

With the above being the default.

sindresorhus · 2019-02-07T16:56:48Z

@futpib Or do you think we should do it like this so it's easier to override individual entries?

{
	defaultExport: {
		path: true,
		chalk: true
	},
	namedExport: {
		util: true,
		lodash: true,
		underscore: true
	}
}

futpib · 2019-02-07T17:19:27Z

I just Google'd "javascript import specifier" and all the sources claim it's the right part, like: nodejs.org/api/esm.html#esm_url_based_paths

I went with what astexplorer and estree call import specifier, and it's the same in the standard. Ok, this is too confusing for a rule name.

futpib · 2019-02-07T17:25:29Z

Or do you think we should do it like this so it's easier to override individual entries?

Yes, I prefer the object variant. Would be consistent with what I chose in #237

sindresorhus · 2019-02-07T17:28:12Z

👍, let's go with the object variant.

rahgurung · 2019-02-08T03:57:29Z

Ok thanks, I will come up with a commit soon.

rahgurung · 2019-02-18T17:07:17Z

I'm stuck on how to pass the configurations, I am working in AST Explorer How do I pass options here? @futpib @sindresorhus

futpib · 2019-02-18T17:14:24Z

@gurungrahul2 Not sure if you can. But you can hardcode them for testing and remove when you are ready to commit the code.

rahgurung · 2019-07-13T11:50:25Z

What would having something like

defaultExport: {
		mymodule: false
	}

mean?
Should mymodule be destructed or false means let it be imported anyway?
@sindresorhus @futpib

futpib · 2019-07-13T23:35:15Z

@gurrrung We are basically modelling a set of strings in JSON here. defaultExport would be a set of module names for which only the default export should be used. Only value true means that the module is in the set.

The benefit of this design is that users can override the build-in defaults. We could merge the user-supplied option with the default one (for example with defaultsDeep). So if we had { util: true, path: true } hardcoded, and a user (who wants to lift the restriction off of util module) passed { util: false }, the result would be { util: false, path: true }, which result in util not being restricted, but all the other hardcoded defaults (in this example, only path) would still apply.

sindresorhus · 2019-09-16T08:02:07Z

@gurrrung Still interested in finishing this?

rahgurung · 2019-09-17T08:21:09Z

Yes, I'm on it.

rahgurung · 2019-09-17T11:00:34Z

Why would Integration tests blow up. 😕

sindresorhus · 2019-09-25T09:33:35Z

@fisker Would you be able to help review this?

fisker · 2019-09-25T11:05:11Z

test/import-style.js

+			errors: [buildError({moduleName: 'util', type: 'defaultImport'})]
+		},
+		{
+			code: 'const foo = myFunction(\'util\')',


Why this is error?

Oh! Thanks for the review. I was a little busy in academic work. Yeah, it shouldn't be error, I'll add the check to throw error only in case of require().

fisker

What is the result of

const promisify = require('utils').promisify;

and dynamic import

(async () => {
  const lodash = (await import('lodash')).default;
})();

(async () => {
  const {default: lodash} = await import('lodash');
})();

(async () => {
  const {chunk} = await import('lodash');
})();

we should add them to tests

fisker · 2019-09-25T11:44:26Z

Should we also check export ... from ...?

export {chunk} from 'utils';
export * as default from 'utils';
export {default as default} from 'lodash';
export {default as lodash} from 'lodash';
export * from 'lodash';

rahgurung · 2019-10-19T13:34:53Z

What is the result of

const promisify = require('utils').promisify;

and dynamic import

(async () => {
  const lodash = (await import('lodash')).default;
})();

(async () => {
  const {default: lodash} = await import('lodash');
})();

(async () => {
  const {chunk} = await import('lodash');
})();

we should add them to tests

that's brilliant. I really couldn't thing of this. Can you give me more insight like what should be the behaviour?

sindresorhus · 2020-02-16T12:09:59Z

Can you give me more insight like what should be the behaviour?

@fisker ^

sindresorhus · 2020-02-16T12:10:11Z

@gurrrung Still interested in finishing this?

fisker · 2020-02-16T12:48:20Z

@gurrrung

const promisify = require('utils').promisify;

I mean we need add test, make sure this pass.

(async () => {
  const lodash = (await import('lodash')).default;
})();

(async () => {
  const {default: lodash} = await import('lodash');
})();

(async () => {
  const {chunk} = await import('lodash');
})();

I'm not sure should we support dynamic import?

@sindresorhus

export ... from and import() support? You should make the call.

I suggest we should support export ... from, import() maybe a little hard , we can ignore that for now.

sindresorhus · 2020-02-16T14:10:38Z

I'm not sure should we support dynamic import?

Nah, not worth the effort. For all our rules, we can stay with the top-level imports.

I suggest we should support export ... from,

Agreed

sindresorhus · 2020-06-21T07:16:28Z

Closing for lack of activity.

rahgurung added 2 commits February 2, 2019 11:07

sindresorhus#211 - Add no-direct-import rule

6815b53

fixed linting errors

cdc9d76

sindresorhus changed the title ~~#211 - Add no-direct-import rule~~ Add no-direct-import rule Feb 2, 2019

sindresorhus requested changes Feb 2, 2019

View reviewed changes

test/no-direct-import.js Outdated Show resolved Hide resolved

docs/rules/no-direct-import.md Outdated Show resolved Hide resolved

readme.md Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

readme.md Outdated Show resolved Hide resolved

rahgurung changed the title ~~Add no-direct-import rule~~ Add import-style rule Feb 11, 2019

Merge branch 'master' into 211

79c8932

sindresorhus force-pushed the master branch from 9266fae to 37e5675 Compare August 19, 2019 18:59

rahgurung added 2 commits September 17, 2019 16:12

feedback commit

764ff40

Merge branch 'master' into 211

4cd9d7d

removed old file

296660d

fisker reviewed Sep 25, 2019

View reviewed changes

sindresorhus closed this Jun 21, 2020

sindresorhus mentioned this pull request Jun 21, 2020

Rule proposal: Don't import util Node.js core module directly #211

Closed

Add import-style rule #232

Add import-style rule #232

Conversation

rahgurung commented Feb 2, 2019 • edited Loading

sindresorhus commented Feb 2, 2019

sindresorhus commented Feb 2, 2019

rahgurung commented Feb 5, 2019

futpib commented Feb 5, 2019

rahgurung commented Feb 6, 2019

sindresorhus commented Feb 6, 2019

rahgurung commented Feb 6, 2019

rahgurung commented Feb 6, 2019

futpib commented Feb 7, 2019

sindresorhus commented Feb 7, 2019 • edited Loading

sindresorhus commented Feb 7, 2019

sindresorhus commented Feb 7, 2019

sindresorhus commented Feb 7, 2019 • edited by futpib Loading

futpib commented Feb 7, 2019

futpib commented Feb 7, 2019

sindresorhus commented Feb 7, 2019 • edited Loading

rahgurung commented Feb 8, 2019

rahgurung commented Feb 18, 2019

futpib commented Feb 18, 2019

rahgurung commented Jul 13, 2019

futpib commented Jul 13, 2019 • edited Loading

sindresorhus commented Sep 16, 2019

rahgurung commented Sep 17, 2019

rahgurung commented Sep 17, 2019

sindresorhus commented Sep 25, 2019

fisker Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

rahgurung Oct 19, 2019

Choose a reason for hiding this comment

fisker left a comment • edited Loading

Choose a reason for hiding this comment

fisker commented Sep 25, 2019 • edited Loading

rahgurung commented Oct 19, 2019 • edited Loading

sindresorhus commented Feb 16, 2020

sindresorhus commented Feb 16, 2020

fisker commented Feb 16, 2020 • edited Loading

sindresorhus commented Feb 16, 2020

sindresorhus commented Jun 21, 2020

Add `import-style` rule #232

Add `import-style` rule #232

rahgurung commented Feb 2, 2019 •

edited

Loading

sindresorhus commented Feb 7, 2019 •

edited

Loading

sindresorhus commented Feb 7, 2019 •

edited by futpib

Loading

sindresorhus commented Feb 7, 2019 •

edited

Loading

futpib commented Jul 13, 2019 •

edited

Loading

fisker Sep 25, 2019 •

edited

Loading

fisker left a comment •

edited

Loading

fisker commented Sep 25, 2019 •

edited

Loading

rahgurung commented Oct 19, 2019 •

edited

Loading

fisker commented Feb 16, 2020 •

edited

Loading