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

exports-last #632

Merged
merged 9 commits into from
Aug 27, 2017
Merged

exports-last #632

merged 9 commits into from
Aug 27, 2017

Conversation

k15a
Copy link
Contributor

@k15a k15a commented Oct 19, 2016

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?

@coveralls
Copy link

coveralls commented Oct 19, 2016

Coverage Status

Coverage increased (+0.05%) to 94.943% when pulling 7e00bb2 on k15a:master into 2cbd801 on benmosher:master.

@k15a k15a mentioned this pull request Oct 22, 2016
@benmosher
Copy link
Member

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).

@k15a
Copy link
Contributor Author

k15a commented Nov 3, 2016

I've just added documentation for this rule. 📖

@coveralls
Copy link

coveralls commented Nov 3, 2016

Coverage Status

Coverage increased (+0.1%) to 94.991% when pulling 037d32b on k15a:master into 2cbd801 on benmosher:master.

@coveralls
Copy link

coveralls commented Nov 12, 2016

Coverage Status

Coverage increased (+0.05%) to 94.907% when pulling dc65882 on k15a:master into 90ef48b on benmosher:master.

@k15a
Copy link
Contributor Author

k15a commented Nov 12, 2016

I've resolved the merge conflicts but now travis ci is failing and I couldn't figure out why.

@k15a
Copy link
Contributor Author

k15a commented Dec 14, 2016

I've removed the CommonJS part. Let me know if I can do something else to get this merged.

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.04%) to 94.904% when pulling 3fcdec1 on k15a:master into 90ef48b on benmosher:master.

Copy link
Member

@benmosher benmosher left a 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 😅

@benmosher
Copy link
Member

LGTM, but would like to have one of @jfmengels or @ljharb take a quick look and sign off as well before merging 👍🏻

Copy link
Collaborator

@jfmengels jfmengels left a 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

@@ -0,0 +1,35 @@
function isExportStatement({ type }) {
// ES Module export statements
if (type === 'ExportDefaultDeclaration' || type === 'ExportNamedDeclaration') {
Copy link
Collaborator

@jfmengels jfmengels Dec 15, 2016

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

create(context) {
return {
Program({ body }) {
const lastNonExportStatement = body.reduce((acc, node, index) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename to lastNonExportStatementIndex


body.forEach((node, index) => {
if (isExportStatement(node) && index < lastNonExportStatement) {

Copy link
Collaborator

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 😅)

}, 0)

body.forEach((node, index) => {
if (isExportStatement(node) && index < lastNonExportStatement) {
Copy link
Collaborator

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.
Copy link
Collaborator

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

@k15a
Copy link
Contributor Author

k15a commented Dec 15, 2016

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.

valid: [
// Empty file
test({
code: '',
Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

@jfmengels jfmengels left a 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!

@@ -0,0 +1,48 @@
# exports-last

This rule reports all export declaration which come before any non-export statements.
Copy link
Collaborator

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.

export const even = 'count'
export const how = 'many'
`,
errors,
Copy link
Collaborator

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'?


const ruleTester = new RuleTester()

const errors = ['Export statements should appear at the end of the file']
Copy link
Collaborator

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')],
      }),

},
}

export default rule
Copy link
Collaborator

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])
Copy link
Collaborator

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.

valid: [
// Empty file
test({
code: '',
Copy link
Collaborator

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.

## This will not be reported

```JS
export const bool = true
Copy link
Collaborator

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

@k15a
Copy link
Contributor Author

k15a commented Jan 25, 2017

Hey @jfmengels,

I think I now solved all the requested changes. Let me know if I can do something else to get this merged.

@coveralls
Copy link

coveralls commented Jan 25, 2017

Coverage Status

Coverage increased (+0.04%) to 94.952% when pulling 0f1c691 on k15a:master into e137b98 on benmosher:master.

@coveralls
Copy link

coveralls commented Jan 25, 2017

Coverage Status

Coverage increased (+0.04%) to 94.952% when pulling 0f1c691 on k15a:master into e137b98 on benmosher:master.

Copy link
Collaborator

@jfmengels jfmengels left a 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 :)

@coveralls
Copy link

coveralls commented Jan 25, 2017

Coverage Status

Coverage increased (+0.04%) to 94.952% when pulling 26cc994 on k15a:master into e137b98 on benmosher:master.

@ljharb
Copy link
Member

ljharb commented Mar 1, 2017

@k15a please rebase latest master, so as to remove all merge commits

@k15a
Copy link
Contributor Author

k15a commented Mar 1, 2017

I tried my best. Did this removed the merge commits?

ljharb
ljharb previously requested changes Mar 1, 2017
Copy link
Member

@ljharb ljharb left a 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])
Copy link
Member

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 :-)

@k15a
Copy link
Contributor Author

k15a commented Mar 1, 2017

Fixed that as well. Hope everything is ok now.

@ljharb ljharb dismissed their stale review March 2, 2017 04:16

looks good; deferring to @benmosher and @jfmengels

@alexkrolick
Copy link

alexkrolick commented May 16, 2017

I'd like to use this rule - anyone want to take a look at it again? Looks reeaaallly close to being mergeable.

@sevor005

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants