-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Changes from 7 commits
7c8e26e
c3a6026
72f882d
7c96d01
574e04a
b3bfe56
016672b
0ec1789
3ec2248
27836ba
db1343f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||
``` | ||
|
||
...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'; | ||
``` |
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if (im.type === 'ImportSpecifier' && im.imported.name === 'default') {
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error is currently: "Using name I'd change the error message to "Use default import syntax to import |
||
'\' as identifier for default export.') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'), | ||
} | ||
}, | ||
} |
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' }), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' } ] }), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 import {default} from './bar'; |
||
], | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foo, { bar }