New: `group-exports` rule #721

robertrossmann · 2017-01-16T22:31:44Z

Adds a new rule which prefers a particular style in exporting named members from modules.

Looking forward to hearing your feedback. Let me know if I have forgotten anything.

ljharb · 2017-01-16T23:16:48Z

Why would this be preferred over prefer-default-export?

Specifically, why would exporting an object be better than named exports?

coveralls · 2017-01-16T23:22:34Z

Coverage increased (+0.04%) to 94.907% when pulling c3fa502 on Alaneor:prefer-single-export into c975742 on benmosher:master.

coveralls · 2017-01-16T23:27:45Z

Coverage increased (+0.04%) to 94.907% when pulling c3fa502 on Alaneor:prefer-single-export into c975742 on benmosher:master.

robertrossmann · 2017-01-16T23:29:08Z

Well, first things first:

const first = true
export {
  first,
}

Here, I am not "exporting an object" - I am simply declaring all named exports at one place. The above is equivalent to

export const first = true

The reason to use this rule primarily comes from some developers exporting at various places in a file. This makes it a bit hard to find out what exactly is exported from a particular module (because you have to scan through the whole file to find all the named exports). Preferring a singular export {} declaration simplifies this.

This rule is not preferred over prefer-default-export. Rather, it seems to be a complement to that rule. There are situations where you do not want to provide a single, default export because the things you export do not necessarily relate, ie. a utils module full of various standalone functions. Using named exports for such functions seems more appropriate.

Also, for commonjs modules exporting on a single place also increases performance because you declare the exported object at once, therefore v8 does not have to create new hidden classes for the exported object every time you assign new property to it. Though I have to admit this perf gain is probably marginal.

ljharb · 2017-01-17T00:22:46Z

Negligent performance theorizing shouldn't be used to inform code decisions, especially since v8 isn't the only place this code will be run.

I could certainly understand a rule that requires that all export statements in a file appear grouped - that seems to achieve your goal without requiring that people export an object (a bag of things), ie, without requiring that people forfeit the benefit of using named exports.

robertrossmann · 2017-01-17T00:27:41Z

I would like to stress out that you are not forfeiting the benefit of named exports by doing export { thing1, thing2 } - these are named exports. See MDN.

ljharb · 2017-01-17T00:29:17Z

Ah, I misunderstood. Your rule isn't suggesting that people switch from multiple named exports to an export of an object - it's suggesting that people consolidate all named exports into a single statement?

robertrossmann · 2017-01-17T00:32:35Z

Yes, that's exactly the aim - to have all the exports in one place so that it's absolutely obvious what is being exported. 🎉

As per your other comment, it could be worthwhile considering implementing a rule which requires all export statements to be grouped. That would be a nice addition to this rule since it would require both named and default exports to be grouped together, further improving readability. But that's for another issue. 😄

ljharb · 2017-01-17T00:35:19Z

Then in that case, I think this is an excellent rule, but that the rule name needs a bit of work :-)

Maybe group-named-exports?

If we wanted to combine this, with the ability to require grouping all exports, then perhaps group-exports taking an object with named and default booleans?

robertrossmann · 2017-01-17T11:50:41Z

Sounds like a good idea!

Do you think it would be useful to give users option to specify if the default export should be before or after the named exports? E.g. default: "before-named|after-named". Alternatively, we can also allow a true value in which case the default export may appear before or after the named exports.

Also, what should happen if named is set to false (no grouping of named exports) and default is enabled? Should the default export appear near the first, last or any named export?

coveralls · 2017-01-17T12:10:37Z

Coverage increased (+0.04%) to 94.907% when pulling 14c3150 on Alaneor:prefer-single-export into c975742 on benmosher:master.

robertrossmann · 2017-01-17T13:59:35Z

I have implemented a simplified version of the default export placement check - for now, it requires the default export to be adjacent to the named export (either before or after it). To make it simple, it does not check the placement if there are still multiple named exports. We may decide how this should behave in such situation (maybe it should then be adjacent to the last named export?).

Let me know what you think so far. We can discuss the options we would like to provide for this rule, then I will extend the current implementation with those options.

Thanks!

coveralls · 2017-01-17T14:09:22Z

Coverage increased (+0.02%) to 94.888% when pulling 57cb453 on Alaneor:prefer-single-export into c975742 on benmosher:master.

coveralls · 2017-01-17T14:22:53Z

Coverage increased (+0.02%) to 94.888% when pulling 57cb453 on Alaneor:prefer-single-export into c975742 on benmosher:master.

ljharb · 2017-01-17T17:08:34Z

tests/src/rules/group-exports.js

+        'export const another = true',
+      ].join('\n'),
+      errors: [
+        'Multiple named export declarations',


let's have this error message contain something actionable - maybe "Multiple named export declarations; consolidate all named exports into a single statement"

Also, could this be autofixable?

Messages updated as requested.

Autofixing would be quite difficult, it seems - we have to

remove export from in front of the things being exported

add one extra export declaration somewhere into the file with all the exported names

The first step seems easy, but I am unsure how to do the second, given you can do only one fix per rule violation.

I guess we could use the last found export and do a full replacement of the source where we append the export at the end...Not sure if this is a good idea, though - if the code we replace contains other issues, I have no idea how ESLint would solve that.

I think this would be pretty hard to auto-fix yes :/

ljharb · 2017-01-17T17:09:26Z

tests/src/rules/group-exports.js

+      errors: [
+        'Multiple CommonJS exports',
+        'Multiple CommonJS exports',
+        'Multiple CommonJS exports',


similarly here, "Multiple CommonJS exports; do a single assignment to module.exports for all of your exports."

Also, what about assigning to exports without the module prefix?

Assignment to the exports object is covered here.

robertrossmann · 2017-01-17T19:37:29Z

I just pushed another update to this PR, incorporating remaining suggestions and fixing some issues:

Assigning to module.exports.exported.* or deeper members is no longer considered as an export
Accessing properties on module.exports is no longer mistakenly considered as an export declaration 😱

coveralls · 2017-01-17T21:05:29Z

Coverage increased (+0.05%) to 94.916% when pulling d4bed97 on Alaneor:prefer-single-export into c975742 on benmosher:master.

ljharb · 2017-01-18T06:58:02Z

docs/rules/group-exports.md

+const first = true
+const second = true
+
+// A single named export -> ok


technically this is a single named export statement but it's two named exports - can we add "statement"?

ljharb · 2017-01-18T06:58:21Z

docs/rules/group-exports.md

+### Invalid
+
+```js
+// Multiple named exports -> not ok!


"Multiple named export statements"

ljharb · 2017-01-18T06:58:58Z

src/rules/group-exports.js

+  ExportDefaultDeclaration:
+    'Default export declaration should be adjacent to named export',
+  MemberExpression:
+    'Multiple CommonJS exports; consolidate all exports into a single assignment to ' +


any reason this isn't on a single line? if max-len is complaining, let's enable the option that ignores long strings.

I put those in a /* eslint-disable max-len */ block.

ljharb · 2017-01-18T06:59:44Z

src/rules/group-exports.js

+          return
+        }
+
+        return void exports.named.add(node)


Why are these using return void instead of doing two things in two statements? Namely, exports.named.add(node); return;

Personal preference, I guess. I like to return together with the last statement in the function. Now they are split into separate lines.

ljharb · 2017-01-18T07:00:24Z

tests/src/rules/group-exports.js

+  default:
+    'Default export declaration should be adjacent to named export',
+  commonjs:
+    'Multiple CommonJS exports; consolidate all exports into a single assignment to ' +


same here re, let's let this be a single line

robertrossmann · 2017-01-18T10:34:52Z

Updated with recent comments.

I pushed it as a separate commit, figured it would be easier to review incrementally than going through the whole thing again and again. Once you are ready to merge, I can squash them all into a single commit.

coveralls · 2017-01-18T11:15:20Z

Coverage increased (+0.06%) to 94.922% when pulling 03225eb on Alaneor:prefer-single-export into c975742 on benmosher:master.

jfmengels · 2017-01-21T09:46:11Z

README.md

@@ -76,6 +76,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
 * Limit the maximum number of dependencies a module can have ([`max-dependencies`])
 * Forbid unassigned imports ([`no-unassigned-import`])
 * Forbid named default exports ([`no-named-default`])
+* Prefer single named export declaration ([`group-exports`])


First of all, thanks @Alaneor! I think the rule idea is interesting :)

Prefer grouping the export of named exports as a single named export declaration?
There may be too many uses of 'export' but I think this is clearer, otherwise people might think it's to limit the number of named exports to one.

Changed to Prefer named exports to be grouped together in a single export declaration

jfmengels · 2017-01-21T09:52:10Z

docs/rules/group-exports.md

@@ -0,0 +1,62 @@
+# group-exports
+
+Reports when multiple named exports or CommonJS assignments are present in a single file and when the default export is not adjacent to the named export.


I think it might be a potentially neat idea to enforce having both the default export and the named exports located next to eah other, but IMO this is out of the scope of the rule. Maybe some people would want to have this enforced but still be allowed to write code like

export const foo = 1; export const bar = 2; export default

IMO, this should be a whole different rule. And it's already being tackled in #632 (it enforces they are the last thing in the file, but the same idea is applied).

Checks about grouping named and default exports next to each other have been removed.

jfmengels · 2017-01-21T09:55:06Z

docs/rules/group-exports.md

@@ -0,0 +1,62 @@
+# group-exports
+
+Reports when multiple named exports or CommonJS assignments are present in a single file and when the default export is not adjacent to the named export.


Reports when multiple named exports . Add something like instead of one grouped named exports declaration. The idea is rather to enforce an alternative way of doing things than straight out forbidding something, and I think the description could be more explicit about that.

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

jfmengels · 2017-01-21T09:55:47Z

docs/rules/group-exports.md

+
+## Rule Details
+
+This rule warns whenever a single file contains multiple named exports or assignments to `module.exports` (or `exports`) and when the default export is not adjacent to the named export.


assignments to a property of module.exports (or exports).

I agree that writing

module.exports = A module.exports = B

is an error, but this should be handled by a different rule (there may already be one, haven't checked yet)

jfmengels · 2017-01-21T10:00:17Z

tests/src/rules/group-exports.js

+      'const first = true',
+      'const second = true',
+      'export { first,\nsecond }',
+    ].join('\n') }),


Instead of joining strings, use template literals. Simplifies writing tests.

test({ code: ` const first = true const second = true export { first, second }` ),

Do the same thing for every test where you have \n.

Updated, thanks!

jfmengels · 2017-01-21T11:55:55Z

docs/rules/group-exports.md

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


This is a it more complicated for CommonJS. If you wish for your module to export a function, but also some other things under "named" exports

module.exports = function foo() {}; module.exports.first = true; module.exports.second = true;

You could potentially do this like in the following example, but that's pretty odd to satisfy this rule.

module.exports = Object.assign(function foo {}, {first: true, second: true});

IMO, this rule should either:

only reports errors for ES modules

only report the use of several instructions like module.exports.foo = true if there is no other direct assignment (module.exports = foo). <-- I'd go for this as I myself prefer grouping module.exports = { foo, bar } when possible.

I have refactored the rule to only do anything if the thing assigned to module.exports is an object literal or there is nothing being assigned to that property. If it's anything else (function, string, what have you) it will not report any errors.

So, this is still an error:

module.exports = {} module.exports.prop = true

But this is now ok:

module.exports = () => {} module.exports.first = true module.exports.second = true

I guess this is the best of both approaches.

What about exports = {} or exports.foo = {}?

@ljharb exports = {} will not export anything nor will it modify whatever value is in module.exports, the exports variable is but a local reference to the default module.exports object.

As for exports.foo = {}, that's basically a named export of an object.

@robertrossmann yes, i'm aware of that; it could still be invalid, but doesn't necessarily have to be covered by this rule.

imo exports.foo = {} should be treated the same as module.exports.foo = {}.

If someone wants to export a function with extra properties, they should do something like const foo = () => {}; foo.first = true; foo.second = true; module.exports = foo; - I don't think the rule should allow multiple module.exports mutations under any circumstances.

@ljharb assignments to exports and module.exports are indeed treated the same. 😄 I have added more tests to validate this.

So

exports.first = true module.exports.second = true

will cause this rule to issue a warning for both lines.

Currently the rule will ignore situations in which you assign something else than an object literal to module.exports (the rule will ignore all CommonJS module operations), so

module.exports = function test() {} module.exports.extra = true module.exports.field = true

will not cause any warnings. Do you think it should warn even in situations like these?

Yes, I do. I don't see why a non-object-literal is any better or worse of a pattern than an object literal here.

Has this been addressed?

Yes - the rule now does not care what is being assigned to module.exports. Some tests for that behaviour can be found here.

jfmengels · 2017-01-21T11:57:47Z

src/rules/group-exports.js

+}
+
+function create(context) {
+  const exports = {


Can you rename this to something else? exports is what I consider a "semi-reserved" keyword in Node, better to name it something else to avoid confusion.

Removed a bunch of now-obsolete code and ultimately renamed the Set to named, since we are now only tracking named exports.

jfmengels · 2017-01-21T12:00:48Z

src/rules/group-exports.js

+
+      if (node.object.name === 'module' && node.property.name === 'exports') {
+        // module.exports.exported.*: assignments this deep should not be considered as exports
+        if (accessorDepth(node) > 2) {


Could you add tests for this? Multiple instructions like module.exports.foo.bar = 1 for instance.

@jfmengels

Deferring to @jfmengels

coveralls · 2017-05-01T11:35:54Z

Coverage increased (+0.05%) to 94.964% when pulling 05aad15 on robertrossmann:prefer-single-export into 106740f on benmosher:master.

robertrossmann · 2017-05-01T11:39:06Z

@jfmengels Thanks for the feedback! I have incorporated all the changes you requested, please have a look.

Since this rule now only does anything useful with named exports, I suggest we rename the rule to group-named-exports.

coveralls · 2017-05-02T11:40:04Z

Coverage increased (+0.1%) to 95.022% when pulling 22e103c on robertrossmann:prefer-single-export into 106740f on benmosher:master.

robertrossmann · 2017-05-02T14:43:07Z

I just pushed a major refactor of the rule's implementation as I discovered some bugs in how CommonJS modules were handled. See 22e103c.

Thank you for your review!

ljharb · 2017-05-02T16:21:45Z

I think "group exports" is still the proper name; since named exports are the only way you could ever have > 1 to group, that was always the case, but exports.foo is not a named export - it's a property on the default export.

robertrossmann · 2017-06-13T16:38:47Z

@ljharb the test suite failed due to coveralls error, looks like something to do with me rebasing the branch. Restarting the suite should fix it. Thanks!

coveralls · 2017-07-10T07:43:40Z

Coverage increased (+0.06%) to 96.022% when pulling fd516ad on robertrossmann:prefer-single-export into c9dd91d on benmosher:master.

robertrossmann · 2017-07-19T18:15:09Z

Closing due to constantly failing tests and an unnecessarily long discussion record and outdated review notes which I fear have been preventing merge for almost 7 months now.

Opened a new issue.

ljharb · 2017-07-19T18:35:55Z

@robertrossmann please don't close PRs and open duplicates; in no way are any of the things you mentioned preventing merges.

Please reopen this and close the duplicate - duplicate PRs clutter the repo forever, since github creates undeleteable refs to them.

robertrossmann · 2017-07-19T18:44:38Z

@ljharb Reopened. Just let me know what needs to be done in order to have this merged, please. Or tell me you don't like it so I don't waste time coming back to this. Thanks!

ljharb · 2017-07-19T18:55:19Z

I do like it; @jfmengels has an outstanding review, and I'd prefer them to either approve or dismiss it before this is merged. If @benmosher approves first, we may be able to bypass that, but it's not ideal.

coveralls · 2017-07-19T21:35:08Z

Coverage increased (+0.06%) to 96.022% when pulling fd516ad on robertrossmann:prefer-single-export into c9dd91d on benmosher:master.

robertrossmann · 2017-08-25T11:19:52Z

Any chance to have this merged or reviewed? Thanks.

robertrossmann · 2017-10-05T12:50:42Z

Rebased again. Could we get this reviewed and merged in?

Regarding tests: They seem to be failing due to an unrelated error in exports-last valid test. See the logs.

Thank you.

coveralls · 2017-10-25T09:44:41Z

Coverage increased (+0.06%) to 96.093% when pulling a5f9d21 on robertrossmann:prefer-single-export into 98acd6a on benmosher:master.

robertrossmann · 2017-10-25T09:46:43Z

Rebased again. Could we get this reviewed and merged in?

@jfmengels you have requested changes on this PR but your feedback has been addressed, I believe.

10 months, plus the changes seem addressed

ljharb

LGTM after a few more changes

ljharb · 2017-10-25T21:02:16Z

CHANGELOG.md

@@ -4,20 +4,13 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).

 ## [Unreleased]
-
-
-## [2.8.0] - 2017-10-18


I think this needs rebasing, and some fixing - it appears some info has been lost

Ooops, sorry - fixed! If you'd like, I can drop the changes to CHANGELOG completely. Not sure if it's supposed to be part of a PR.

nah it's good to add it to "unreleased"

ljharb · 2017-10-25T21:02:58Z

docs/rules/group-exports.md

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


Has this been addressed?

ljharb · 2017-10-25T21:03:33Z

src/rules/group-exports.js

+    'Program:exit': function onExit() {
+      // Report multiple `export` declarations (ES2015 modules)
+      if (nodes.modules.size > 1) {
+        for (const node of nodes.modules) {


could we use nodes.modules.forEach here, instead of a loop?

ljharb · 2017-10-25T21:03:37Z

src/rules/group-exports.js

+
+      // Report multiple `module.exports` assignments (CommonJS)
+      if (nodes.commonjs.size > 1) {
+        for (const node of nodes.commonjs) {


robertrossmann · 2017-10-26T09:31:34Z

@ljharb thanks for feedback - I think I have made all the requested changes.

coveralls · 2017-10-26T10:14:30Z

Coverage increased (+0.06%) to 96.093% when pulling 365dfbb on robertrossmann:prefer-single-export into 98acd6a on benmosher:master.

robertrossmann · 2017-10-26T10:16:06Z

Looks like AppVeyor tests pass but uploading coverage data failed on all jobs and marked the build as failed.

ljharb · 2017-10-27T01:09:31Z

Let's give this a few more days, but then I'm going to merge it.

robertrossmann mentioned this pull request Jan 16, 2017

Rule to check module.exports consistency strvcom/eslint-config-javascript#16

Closed

ljharb added the rule proposal label Jan 17, 2017

robertrossmann changed the title ~~New: prefer-single-export rule~~ New: group-exports rule Jan 17, 2017

ljharb requested changes Jan 17, 2017

View reviewed changes

ljharb previously requested changes Jan 18, 2017

View reviewed changes

jfmengels previously requested changes Jan 21, 2017

View reviewed changes

robertrossmann closed this Jul 19, 2017

robertrossmann deleted the prefer-single-export branch July 19, 2017 18:07

robertrossmann mentioned this pull request Jul 19, 2017

New: group-exports rule #898

Closed

ljharb marked this as a duplicate of #898 Jul 19, 2017

robertrossmann reopened this Jul 19, 2017

ljharb requested a review from benmosher July 19, 2017 18:43

ljharb requested changes Oct 25, 2017

View reviewed changes

New: group-exports rule

365dfbb

ljharb approved these changes Oct 27, 2017

View reviewed changes

ljharb merged commit eca7b4d into import-js:master Oct 31, 2017

ljharb added accepted semver-minor labels Oct 31, 2017

renovate bot mentioned this pull request Feb 22, 2018

Update dependency eslint-plugin-import to v2.11.0 Urigo/tortilla#68

Merged

renovate bot mentioned this pull request Mar 22, 2018

Update eslint-plugin-import to 2.12.0 sbardian/DebtTrackerAPI#25

Merged

		@@ -0,0 +1,62 @@
		# group-exports

		Reports when multiple named exports or CommonJS assignments are present in a single file and when the default export is not adjacent to the named export.


		## Rule Details

		This rule warns whenever a single file contains multiple named exports or assignments to `module.exports` (or `exports`) and when the default export is not adjacent to the named export.

New: group-exports rule #721

New: group-exports rule #721

Conversation

robertrossmann commented Jan 16, 2017

ljharb commented Jan 16, 2017

coveralls commented Jan 16, 2017 • edited Loading

coveralls commented Jan 16, 2017 • edited Loading

robertrossmann commented Jan 16, 2017

ljharb commented Jan 17, 2017

robertrossmann commented Jan 17, 2017

ljharb commented Jan 17, 2017

robertrossmann commented Jan 17, 2017

ljharb commented Jan 17, 2017

robertrossmann commented Jan 17, 2017

coveralls commented Jan 17, 2017 • edited Loading

robertrossmann commented Jan 17, 2017

coveralls commented Jan 17, 2017 • edited Loading

coveralls commented Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrossmann commented Jan 17, 2017

coveralls commented Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrossmann commented Jan 18, 2017

coveralls commented Jan 18, 2017 • edited Loading

jfmengels Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 1, 2017 • edited Loading

robertrossmann commented May 1, 2017

coveralls commented May 2, 2017 • edited Loading

robertrossmann commented May 2, 2017

ljharb commented May 2, 2017

robertrossmann commented Jun 13, 2017

coveralls commented Jul 10, 2017 • edited Loading

robertrossmann commented Jul 19, 2017

ljharb commented Jul 19, 2017

robertrossmann commented Jul 19, 2017

ljharb commented Jul 19, 2017

coveralls commented Jul 19, 2017 • edited Loading

robertrossmann commented Aug 25, 2017

robertrossmann commented Oct 5, 2017

coveralls commented Oct 25, 2017 • edited Loading

robertrossmann commented Oct 25, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

New: `group-exports` rule #721

New: `group-exports` rule #721

coveralls commented Jan 16, 2017 •

edited

Loading

coveralls commented Jan 16, 2017 •

edited

Loading

coveralls commented Jan 17, 2017 •

edited

Loading

coveralls commented Jan 17, 2017 •

edited

Loading

coveralls commented Jan 17, 2017 •

edited

Loading

coveralls commented Jan 17, 2017 •

edited

Loading

coveralls commented Jan 18, 2017 •

edited

Loading

jfmengels Jan 21, 2017 •

edited

Loading

coveralls commented May 1, 2017 •

edited

Loading

coveralls commented May 2, 2017 •

edited

Loading

coveralls commented Jul 10, 2017 •

edited

Loading

coveralls commented Jul 19, 2017 •

edited

Loading

coveralls commented Oct 25, 2017 •

edited

Loading

coveralls commented Oct 26, 2017 •

edited

Loading