-
-
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
exports-last #632
exports-last #632
Conversation
The ES module part looks good to me. 👍 I'd leave out CommonJS for now and add it later behind a flag (unless it's a use case you're looking forward to linting). |
I've just added documentation for this rule. 📖 |
I've resolved the merge conflicts but now travis ci is failing and I couldn't figure out why. |
I've removed the CommonJS part. Let me know if I can do something else to get this merged. |
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.
looks good to me! sorry this has been parked for so long. bad timing 😅
LGTM, but would like to have one of @jfmengels or @ljharb take a quick look and sign off as well before merging 👍🏻 |
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.
Hey @k15a! Thanks for the PR, and sorry for the delay in answering this.
This looks pretty good. I made a few comments, and would love a few more tests.
- Support for the export all declaration (
export * from 'foo'
). Which, by the way, might be more interesting to allow that to be on top as it's also kind of an import (happy to discuss that further). It needs to be either tested or documented as an exception IMO. - More test cases:
- complex cases with plenty (3-5) of exports and some normal case in between
- empty file with no statements (trying to make sure this doesn't crash now or in the future)
- file with only exports
src/rules/exports-last.js
Outdated
@@ -0,0 +1,35 @@ | |||
function isExportStatement({ type }) { | |||
// ES Module export statements | |||
if (type === 'ExportDefaultDeclaration' || type === 'ExportNamedDeclaration') { |
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.
Simpler:
function isExportStatement({ type }) {
return type === 'ExportDefaultDeclaration' || type === 'ExportNamedDeclaration';
}
And you should probably also test for ExportAllDeclaration (and add tests for that case), but happy to discuss that as an exception
src/rules/exports-last.js
Outdated
create(context) { | ||
return { | ||
Program({ body }) { | ||
const lastNonExportStatement = body.reduce((acc, node, index) => { |
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.
rename to lastNonExportStatementIndex
src/rules/exports-last.js
Outdated
|
||
body.forEach((node, index) => { | ||
if (isExportStatement(node) && index < lastNonExportStatement) { | ||
|
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.
remove this blank line (sorry about the nitpicking 😅)
src/rules/exports-last.js
Outdated
}, 0) | ||
|
||
body.forEach((node, index) => { | ||
if (isExportStatement(node) && index < lastNonExportStatement) { |
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.
It might be a bit faster to do the index < lastNonExportStatement
before the isExportStatement
.
Or even better (?): only run the forEach
on the statements after lastNonExportStatement
.
// maybe add +/-1 to lastNonExportStatement to make it work right
body.slice(lastNonExportStatement).forEach(node => {
if (isExportStatement(node)) {
// report
}
})
|
||
## When Not To Use It | ||
|
||
If you don't mind exports being sprinkled throughout a file, you may not want to enable this rule. |
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.
It would be nice to indicate that this is only for the export statements using the export
keyword, and not for CommonJS module.exports/exports
Hey @jfmengels, I've added some more tests unfortunately one test is breaking but I think that's not an issue with my rule. I've also refactored the code. Let me know if I can do something else. |
tests/src/rules/exports-last.js
Outdated
valid: [ | ||
// Empty file | ||
test({ | ||
code: '', |
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 line is breaking the tests.
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.
Ok, try replacing it with a comment
test({
code: '// comment',
})
and if that doesn't work out, just remove the test case.
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.
Hey @k15a !
Sorry for the late reply, totally my fault...
I still have a few comments about the rule. If you're still up for making changes, that'd be great!
docs/rules/exports-last.md
Outdated
@@ -0,0 +1,48 @@ | |||
# exports-last | |||
|
|||
This rule reports all export declaration which come before any non-export statements. |
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 rule enforces that all exports are declared at the bottom of the file. This rule will report any export declarations that comes before any non-export statements.
tests/src/rules/exports-last.js
Outdated
export const even = 'count' | ||
export const how = 'many' | ||
`, | ||
errors, |
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.
Shouldn't there be 2 errors here? One for the export default
and one for export const so = 'many'
?
tests/src/rules/exports-last.js
Outdated
|
||
const ruleTester = new RuleTester() | ||
|
||
const errors = ['Export statements should appear at the end of the file'] |
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.
const error = {
ruleId: 'exports-last',
message: 'Export statements should appear at the end of the file',
};
You can make this into an array by default but I think you'll need to make an array of multiple errors for some tests.
Would be nice if you could add:
- either the location of the reported node
- the type of the Node :
type: 'ImportDefaultSpecifier
. This might be easier.
At the moment, you are not reporting the export statements, but non-export statements. Any would be fine (though I' prefer reporting the export statements), but the message mentions the export statement, so let's report those. That's why I'd like to add this additional verification.
It could look something like:
const error = type => {
ruleId: 'exports-last',
message: 'Export statements should appear at the end of the file',
type
};
// ------
test({
code: `
export * from './foo'
const bar = true
`,
errors: [error('ExportAllDeclaration')],
}),
src/rules/exports-last.js
Outdated
}, | ||
} | ||
|
||
export default rule |
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.
Would you mind making the file structure (exports and stuff) look a bit more like the others, for consistency? Here's an example: https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-dynamic-require.js
CHANGELOG.md
Outdated
@@ -26,6 +26,9 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel | |||
- Properly report [`newline-after-import`] when next line is a decorator | |||
- Fixed documentation for the default values for the [`order`] rule ([#601]) | |||
|
|||
### Added | |||
- [`exports-last`] lints that export statements are at the end of the file ([#620] + [#632]) |
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.
You'll need to rebase and move this back into the Unreleased
section. Things have changed ever since you mad your PR 😅
If you don't know how to rebase, don't mind it, I'll do it later for you.
tests/src/rules/exports-last.js
Outdated
valid: [ | ||
// Empty file | ||
test({ | ||
code: '', |
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.
Ok, try replacing it with a comment
test({
code: '// comment',
})
and if that doesn't work out, just remove the test case.
docs/rules/exports-last.md
Outdated
## This will not be reported | ||
|
||
```JS | ||
export const bool = 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.
Add a simple non-exported variable declaration up top, to show that that is allowed
Hey @jfmengels, I think I now solved all the requested changes. Let me know if I can do something else to get this merged. |
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.
LGTM, thanks @k15a!
I'm giving the others a bit time to review this :)
@k15a please rebase latest master, so as to remove all merge commits |
exports-last will check that all export statements are at the end of the file Closes #620
I tried my best. Did this removed the merge commits? |
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.
Rebase looks good except for one thing :-)
CHANGELOG.md
Outdated
@@ -6,8 +6,8 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel | |||
## [Unreleased] | |||
### Added | |||
- [`no-anonymous-default-export`] rule: report anonymous default exports ([#712], thanks [@duncanbeevers]). | |||
- Add new value to [`order`]'s `newlines-between` option to allow newlines inside import groups ([#627], [#628], thanks [@giodamelio]) | |||
- Add `count` option to the [`newline-after-import`] rule to allow configuration of number of newlines expected ([#742], thanks [@ntdb]) |
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.
we lost one of the changelog entries - let's fix that :-)
Fixed that as well. Hope everything is ok now. |
looks good; deferring to @benmosher and @jfmengels
I'd like to use this rule - anyone want to take a look at it again? Looks reeaaallly close to being mergeable. |
Hey,
I am currently working on #620 and would like some feedback.
Currently my implementation only checks for ES Modules export declaration but I would also like to support CommonJS modules as well. Does anybody has an idea how to check if an ExpressionStatement is an CommonJS export?