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

New: group-exports rule #721

Merged
merged 1 commit into from
Oct 31, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel

## [Unreleased]

- Add [`group-exports`] rule: style-guide rule to report use of multiple named exports ([#721], thanks [@robertrossmann])

## [2.8.0] - 2017-10-18
### Added
Expand Down Expand Up @@ -431,6 +432,7 @@ for info on changes for earlier releases.
[`unambiguous`]: ./docs/rules/unambiguous.md
[`no-anonymous-default-export`]: ./docs/rules/no-anonymous-default-export.md
[`exports-last`]: ./docs/rules/exports-last.md
[`group-exports`]: ./docs/rules/group-exports.md

[`memo-parser`]: ./memo-parser/README.md

Expand All @@ -442,6 +444,7 @@ for info on changes for earlier releases.
[#744]: https://github.com/benmosher/eslint-plugin-import/pull/744
[#742]: https://github.com/benmosher/eslint-plugin-import/pull/742
[#737]: https://github.com/benmosher/eslint-plugin-import/pull/737
[#721]: https://github.com/benmosher/eslint-plugin-import/pull/721
[#712]: https://github.com/benmosher/eslint-plugin-import/pull/712
[#696]: https://github.com/benmosher/eslint-plugin-import/pull/696
[#685]: https://github.com/benmosher/eslint-plugin-import/pull/685
Expand Down Expand Up @@ -661,3 +664,4 @@ for info on changes for earlier releases.
[@mplewis]: https://github.com/mplewis
[@rosswarren]: https://github.com/rosswarren
[@alexgorbatchev]: https://github.com/alexgorbatchev
[@robertrossmann]: https://github.com/robertrossmann
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
* Forbid unassigned imports ([`no-unassigned-import`])
* Forbid named default exports ([`no-named-default`])
* Forbid anonymous values as default exports ([`no-anonymous-default-export`])
* Prefer named exports to be grouped together in a single export declaration ([`group-exports`])

[`first`]: ./docs/rules/first.md
[`exports-last`]: ./docs/rules/exports-last.md
Expand All @@ -91,6 +92,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
[`no-unassigned-import`]: ./docs/rules/no-unassigned-import.md
[`no-named-default`]: ./docs/rules/no-named-default.md
[`no-anonymous-default-export`]: ./docs/rules/no-anonymous-default-export.md
[`group-exports`]: ./docs/rules/group-exports.md

## Installation

Expand Down
87 changes: 87 additions & 0 deletions docs/rules/group-exports.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# group-exports

Reports when named exports are not grouped together in a single `export` declaration or when multiple assignments to CommonJS `module.exports` or `exports` object are present in a single file.

**Rationale:** An `export` declaration or `module.exports` assignment can appear anywhere in the code. By requiring a single export declaration all your exports will remain at one place, making it easier to see what exports a module provides.

## Rule Details

This rule warns whenever a single file contains multiple named export declarations or multiple assignments to `module.exports` (or `exports`).
Copy link
Member

Choose a reason for hiding this comment

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

doesn't it only warn when this is true and when they're not grouped together lexically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do not need to be grouped lexically (I assume that means that the statements are consecutive), there must really be only a single assignment or export declaration.

Copy link
Member

Choose a reason for hiding this comment

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

ah ok, thanks for clarifying.

In that case, is there a reason this rule couldn't be autofixable? (ie, delete all but the last export statements, and replace the last one with the combined group)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not currently see any issues with auto-fixing, either, but I have not included a fixer in this PR for several reasons:

  • I am kind of new to ESLint rule development
  • I have no prior experience manipulating or generally working with AST
  • I wanted to keep this small and push it out fast (even though I then got lost in other tasks and forgot about this PR for several months 😄 )

So I'd rather not add fixer at this moment, even though it should be possible to implement it, eventually.


### Valid

```js
// A single named export declaration -> ok
export const valid = true
```

```js
const first = true
const second = true

// A single named export declaration -> ok
export {
first,
second,
}
```

```js
// A single exports assignment -> ok
module.exports = {
first: true,
second: true
}
```

```js
const first = true
const second = true

// A single exports assignment -> ok
module.exports = {
first,
second,
}
```

```js
function test() {}
test.property = true
test.another = true

// A single exports assignment -> ok
module.exports = test
```


### Invalid

```js
// Multiple named export statements -> not ok!
export const first = true
export const second = true
```

```js
// Multiple exports assignments -> not ok!
exports.first = true
exports.second = true
```

```js
// Multiple exports assignments -> not ok!
module.exports = {}
module.exports.first = true
```

```js
// Multiple exports assignments -> not ok!
module.exports = () => {}
module.exports.first = true
module.exports.second = true
```

## When Not To Use It

If you do not mind having your exports spread across the file, you can safely turn this rule off.
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export const rules = {
'extensions': require('./rules/extensions'),
'no-restricted-paths': require('./rules/no-restricted-paths'),
'no-internal-modules': require('./rules/no-internal-modules'),
'group-exports': require('./rules/group-exports'),

'no-named-default': require('./rules/no-named-default'),
'no-named-as-default': require('./rules/no-named-as-default'),
Expand Down
98 changes: 98 additions & 0 deletions src/rules/group-exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
const meta = {}
/* eslint-disable max-len */
const errors = {
ExportNamedDeclaration: 'Multiple named export declarations; consolidate all named exports into a single export declaration',
AssignmentExpression: 'Multiple CommonJS exports; consolidate all exports into a single assignment to `module.exports`',
}
/* eslint-enable max-len */

/**
* Returns an array with names of the properties in the accessor chain for MemberExpression nodes
*
* Example:
*
* `module.exports = {}` => ['module', 'exports']
* `module.exports.property = true` => ['module', 'exports', 'property']
*
* @param {Node} node AST Node (MemberExpression)
* @return {Array} Array with the property names in the chain
* @private
*/
function accessorChain(node) {
const chain = []

do {
chain.unshift(node.property.name)

if (node.object.type === 'Identifier') {
chain.unshift(node.object.name)
break
}

node = node.object
} while (node.type === 'MemberExpression')

return chain
}

function create(context) {
const nodes = {
modules: new Set(),
commonjs: new Set(),
}

return {
ExportNamedDeclaration(node) {
nodes.modules.add(node)
},

AssignmentExpression(node) {
if (node.left.type !== 'MemberExpression') {
return
}

const chain = accessorChain(node.left)

// Assignments to module.exports
// Deeper assignments are ignored since they just modify what's already being exported
// (ie. module.exports.exported.prop = true is ignored)
if (chain[0] === 'module' && chain[1] === 'exports' && chain.length <= 3) {
nodes.commonjs.add(node)
return
}

// Assignments to exports (exports.* = *)
if (chain[0] === 'exports' && chain.length === 2) {
nodes.commonjs.add(node)
return
}
},

'Program:exit': function onExit() {
// Report multiple `export` declarations (ES2015 modules)
if (nodes.modules.size > 1) {
nodes.modules.forEach(node => {
context.report({
node,
message: errors[node.type],
})
})
}

// Report multiple `module.exports` assignments (CommonJS)
if (nodes.commonjs.size > 1) {
nodes.commonjs.forEach(node => {
context.report({
node,
message: errors[node.type],
})
})
}
},
}
}

export default {
meta,
create,
}
Loading