Add no-named-default rule
#596
Conversation
| }, | ||
|
|
||
| create: function (context) { | ||
| function checkSpecifiers(type, node) { |
There was a problem hiding this comment.
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) { ... } }
| node.specifiers.forEach(function (im) { | ||
| if (im.type !== type) return | ||
|
|
||
| if (im.imported.name === 'default') { |
There was a problem hiding this comment.
if (im.type === 'ImportSpecifier' && im.imported.name === 'default') {
}There was a problem hiding this comment.
I also went ahead and removed the preceding checks as I don't believe they were helping us anymore.
| if (im.imported.name === 'default') { | ||
| context.report(im.local, | ||
| 'Using name \'' + im.local.name + | ||
| '\' as identifier for default export.') |
There was a problem hiding this comment.
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.
| 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.
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';|
|
||
| // 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.
What are the differences with the previous non-es7 test cases? Are you just testing the parsers?
There was a problem hiding this comment.
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.
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).
| ...these would be valid: | ||
| ```js | ||
| import foo from './foo.js'; | ||
| import foo { bar } from './foo.js'; |
|
|
||
| if (im.imported.name === 'default') { | ||
| context.report(im.local, | ||
| 'Using name \'' + im.local.name + |
There was a problem hiding this comment.
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".
| create: function (context) { | ||
| return { | ||
| 'ImportDeclaration': function (node) { | ||
| if (node.source == null) return |
There was a problem hiding this comment.
I'm curious, is there ever a case where the source is not defined? ❓
We can leave it in case you're not sure, but I don't when this would happen.
There was a problem hiding this comment.
TBH, this is here because other rules included it (named, no-deprecated). It now seems likely that it's not needed here, will remove.
| if (im.type === 'ImportSpecifier' && im.imported.name === 'default') { | ||
| context.report({ | ||
| node: im.local, | ||
| message: 'Use default import syntax to ' + |
There was a problem hiding this comment.
You could use template literal syntax here by the way, makes it a bit simpler (and I think you're allowed to put this all on one line, but let me know if you aren't)
`Use default import syntax to import '${im.local.name}'`There was a problem hiding this comment.
Good thought! I'm used to an 80-char line length, this is definitely under the project's max of 99. :)
|
@jfmengels I've just pushed changes based on your most recent review. Cheers! |
|
Awesome, thanks @ntdb! :) |
|
👌🏻 Thanks for this, @ntdb! Cool to see you contributing! And thanks for all the back and forth working together from both you and @jfmengels! |
…mport # Conflicts: # CHANGELOG.md
|
@benmosher Sure thing, thanks for maintaining a great plugin! I've just rebased, should be good to go. |
👋
This is an attempt at building the rule requested in #481. I'm not new to ES2015 or ESLint but I may have misstepped on some terminology and certainly on some logic. Please take a look and let me know what I can improve!