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

Add no-named-default rule #596

Merged
merged 11 commits into from
Nov 2, 2016
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).

## [Unreleased]
- Add [`no-named-default`] rule: style-guide rule to report use of unnecessarily named default imports


## [2.0.0]! - 2016-09-30
Expand Down
6 changes: 4 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
* Enforce a convention in module import order ([`order`])
* Enforce a newline after import statements ([`newline-after-import`])
* Prefer a default export if module exports a single name ([`prefer-default-export`])
* Limit the maximum number of dependencies a module can have. ([`max-dependencies`])
* Forbid unassigned imports. ([`no-unassigned-import`])
* Limit the maximum number of dependencies a module can have ([`max-dependencies`])
* Forbid unassigned imports ([`no-unassigned-import`])
* Forbid named default exports ([`no-named-default`])

[`first`]: ./docs/rules/first.md
[`no-duplicates`]: ./docs/rules/no-duplicates.md
Expand All @@ -85,6 +86,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
[`prefer-default-export`]: ./docs/rules/prefer-default-export.md
[`max-dependencies`]: ./docs/rules/max-dependencies.md
[`no-unassigned-import`]: ./docs/rules/no-unassigned-import.md
[`no-named-default`]: ./docs/rules/no-named-default.md

## Installation

Expand Down
30 changes: 30 additions & 0 deletions docs/rules/no-named-default.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# no-named-default

Reports use of a default export as a locally named import.

Rationale: the syntax exists to import default exports expressively, let's use it

- *misleading*: consistent syntax usage is less likely to confuse new developers
- *a mistake*: meant to import a named export and unintentionally specified `default`

## Rule Details

Given:
```js
// foo.js
export default 'foo';
export const bar = 'baz';
```

...these would be valid:
```js
import foo from './foo.js';
import foo { bar } from './foo.js';
Copy link
Collaborator

Choose a reason for hiding this comment

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

foo, { bar }

```

...and these would be reported:
```js
// message: Using exported name 'bar' as identifier for default export.
import { default as foo } from './foo.js';
import { default as foo, bar } from './foo.js';
```
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export const rules = {
'no-restricted-paths': require('./rules/no-restricted-paths'),
'no-internal-modules': require('./rules/no-internal-modules'),

'no-named-default': require('./rules/no-named-default'),
'no-named-as-default': require('./rules/no-named-as-default'),
'no-named-as-default-member': require('./rules/no-named-as-default-member'),

Expand Down
32 changes: 32 additions & 0 deletions src/rules/no-named-default.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
module.exports = {
meta: {
docs: {},
},

create: function (context) {
function checkSpecifiers(type, node) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no reason to specify the type IMO, as I don't think this code will be extended to account for other types. You should remove the type parameter and the call to bind below. To make things a little bit clearer, you could simply assign this function to ImportDeclaration directly (like { ImportDeclaration: function (node) { ... } }

if (node.source == null) return

const hasImportSpecifier = node.specifiers.some(function (im) {
return im.type === type
})

if (!hasImportSpecifier) {
return
}

node.specifiers.forEach(function (im) {
if (im.type !== type) return

if (im.imported.name === 'default') {
Copy link
Collaborator

@jfmengels jfmengels Oct 1, 2016

Choose a reason for hiding this comment

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

if (im.type === 'ImportSpecifier' && im.imported.name === 'default') {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also went ahead and removed the preceding checks as I don't believe they were helping us anymore.

context.report(im.local,
'Using name \'' + im.local.name +
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error is currently: "Using name bar as identifier for default export".
Technically, if you do import bar from './foo', bar is also the name of the default export, but that's valid.

I'd change the error message to "Use default import syntax to import bar".

'\' as identifier for default export.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

context.report({
  node: im.local,
  message: 'Using name...'
})

Both syntaxes are valid but I think the first one was deprecated. Anyhow, this would be more consistent with the other rules in this repo AFAIK.

}
})
}
return {
'ImportDeclaration': checkSpecifiers.bind(null, 'ImportSpecifier'),
}
},
}
31 changes: 31 additions & 0 deletions tests/src/rules/no-named-default.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { test, SYNTAX_CASES } from '../utils'
import { RuleTester } from 'eslint'

const ruleTester = new RuleTester()
, rule = require('rules/no-named-default')

ruleTester.run('no-named-default', rule, {
valid: [
test({code: 'import bar from "./bar";'}),
test({code: 'import bar, { foo } from "./bar";'}),

// es7
test({ code: 'import bar from "./bar";', parser: 'babel-eslint' }),
test({ code: 'import bar, { foo } from "./bar";', parser: 'babel-eslint' }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the differences with the previous non-es7 test cases? Are you just testing the parsers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup these are here because I was unsure about whether the parser required testing for each rule. Are they safe to remove?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say it's safe yes. At this point and AFAIK, the only difference is that one parser might consider some syntax invalid while another accepts it (and it goes both ways now with the latest version of Espree).


...SYNTAX_CASES,
],

invalid: [
test({
code: 'import { default as bar } from "./bar";',
errors: [ {
message: 'Using name \'bar\' as identifier for default export.'
, type: 'Identifier' } ] }),
test({
code: 'import { foo, default as bar } from "./bar";',
errors: [ {
message: 'Using name \'bar\' as identifier for default export.'
, type: 'Identifier' } ] }),
Copy link
Collaborator

@jfmengels jfmengels Oct 1, 2016

Choose a reason for hiding this comment

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

Could you add the following invalid test case? From what I can see, that looks like a syntax error using the default parser, but still valid with babel-eslint.

import {default} from './bar';

],
})