-
-
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
Filter out default export in an ExportAllDeclaration #331
Conversation
@@ -50,7 +50,13 @@ module.exports = function (context) { | |||
return | |||
} | |||
let any = false | |||
remoteExports.forEach((v, name) => (any = true) && addNamed(name, node)) | |||
remoteExports.forEach((v, name) => ( | |||
(any = true) && |
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.
I know it was already in the original code (so this is more like a question to @benmosher), but wouldn't it be simpler & less odd (an assignation in a condition? oô), to do const any = remoteExports.length > 0
?
I would actually split it up somewhat like this
const namedExports = remoteExports.filter((v, name) => name !== 'default');
const anyNamed = namedExports.length > 0;
namedExports.forEach((v, name) => addNamed(name, node));
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.
@jfmengels wouldn't quite work since any
is used for the No named exports found in module
check below.
This would work though:
const anyNamed = remoteExports.length > 0;
const namedExports = remoteExports.filter((v, name) => name !== 'default');
namedExports.forEach((v, name) => addNamed(name, node));
Happy to change if you think it makes it more readable (I admit I was confused about the assignment in the condition)
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.
FWIW, remoteExports
is more of a Map
than an Array
, so it has forEach
but no filter
. It has a size
, but this size includes the default
name.
I agree that the (any = true)
assignment expression is a little laconic, but it gets the job done.
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.
This is what I ended up with, here: https://github.com/benmosher/eslint-plugin-import/pull/332/files#diff-71778249fbd501cbc801fa0ee1cac9a2R53
default
shouldn't count, so it does trigger the no exported names
report if you export * from 'foo'
where foo
has only a default
export.
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.
That makes sense! I'll close this PR then.
@jkimbo I think the reason I was thinking it's more complicated is that it's larger than this. I.E. right now So it's a larger issue than just the I'm going to take a stab at it from inside the export map stuff, let's pause on this for a second. |
Superseded by #332 |
Fix for #328
@benmosher the fix seems suspiciously simple! Are there any potential problems you can see? Any additional tests that would ensure it's working as expected?