Add `no-default-export` + docs/tests #936

dead-claudia · 2017-10-01T00:08:50Z

Fixes #889

dead-claudia · 2017-10-09T21:21:01Z

The failing tests appear to be irrelevant (and the same error that appears in master).

j-f1

Looks awesome! I can’t wait to put this to use! Just a few suggestions and questions.

j-f1 · 2017-10-16T15:32:47Z

tests/src/rules/no-default-export.js

+      parser: 'babel-eslint',
+      }),
+
+    // ...SYNTAX_CASES,


What does this mean?

It was more or less adapted from prefer-default-export's tests.

j-f1 · 2017-10-16T15:41:51Z

README.md

@@ -91,6 +91,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
+[`no-default-export`]: ./docs/rules/no-default-export.md


You should mention this under one of the headers so it is visible in the README.

Good catch! I completely missed that.

j-f1 · 2017-10-16T15:42:46Z

src/rules/no-default-export.js

+              node.exported.name === 'default') {
+            context.report({
+              node: ast,
+              message: 'Prefer named exports.',


Can you extract this message to an external variable? Naming it message would it be destructured in.

j-f1 · 2017-10-16T15:43:13Z

src/rules/no-default-export.js

+    }
+
+    return {
+      ExportDefaultDeclaration: function (ast) {


Can you call this node to match the other rules?

j-f1 · 2017-10-16T15:43:34Z

src/rules/no-default-export.js

@@ -0,0 +1,40 @@
+module.exports = {
+  meta: {
+    docs: {},


Can you put a description and category in here?

I didn't include it because it seemed that none of the other rules appeared to use it (example).

j-f1 · 2017-10-16T15:46:08Z

tests/src/rules/no-default-export.js

+      }),
+
+    // ...SYNTAX_CASES,
+  ],


Can you add a test for something like import defaultImport from 'module-from-npm'? This should be legal, since we can’t control how other libraries are structured.

I didn't add a test for it, since it only checks export and export ... from. I can add a test to clarify this, though.

coveralls · 2017-10-17T02:55:55Z

Coverage decreased (-0.02%) to 96.012% when pulling 5f9e0ce on isiahmeadows:no-default-import into cef88f2 on benmosher:master.

dead-claudia · 2017-11-26T13:42:16Z

Ping, eslint-plugin-import people? (I feel this might have gotten lost in the weeds a little bit...)

ljharb · 2017-11-26T21:32:03Z

@isiahmeadows if you rebase this (rather than using the "update branch" button), it should rerun the travis tests.

I'll give it a review, but I'm not a fan of even the existence of this rule, so I'm going to leave it to the other collaborators to review/merge.

j-f1 · 2017-11-26T21:37:21Z

CHANGELOG.md

@@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel

 ## [2.8.0] - 2017-10-18
 ### Added
+- [`no-default-export`] rule ([#889], thanks [@isiahmeadows])


Don’t forget to add [@isiahmeadows]: https://github.com/isiahmeadows to the bottom of this file.

Oh yeah...oops

ljharb · 2017-11-26T21:32:37Z

docs/rules/no-default-export.md

@@ -0,0 +1,63 @@
+# no-default-export


i think maybe forbid-default-export would be a more typical name for a rule like this, and for the opposite of "prefer"

I was following the naming convention the other rules have - there's also no-dynamic-require, no-named-as-default, and no-anonymous-default-export. So IMHO it wouldn't make sense to use forbid-${name} when all the others use no-. Also, ESLint recommends this convention, so I'm kind of hesitant to deviate from it

BTW, I kind of missed these comments somehow; sorry about that. 😦

fair enough; this one isn't a blocker or anything.

ljharb · 2017-11-26T21:37:57Z

tests/src/rules/no-default-export.js

+    test({
+      code: 'export { a, b } from "foo.js"',
+      parser: 'babel-eslint',
+      }),


all these }), need to be de-indented a level (it seems like this is already the way it is in the prefer-default-export tests, but let's fix it here anyways)

the indentation here is still off; the } should match the indentation level of the line that ends with }

ljharb · 2017-11-26T21:49:19Z

src/rules/no-default-export.js

+      return {}
+    }
+
+    var message = 'Prefer named exports.'


this should be a const.

also, i wonder if this is an opportunity here to make the message a bit more explanatory?

Will do. I'll just note that the code base is a haphazard mix of var, let, and const, and ES6 features are used very inconsistently.

also, i wonder if this is an opportunity here to make the message a bit more explanatory?

For the case of export {foo as default}, sure. For the rest, I'm not sure there is more that needs to be done IMHO. There's not really any extra context needed.

dead-claudia · 2017-11-27T04:32:10Z

@ljharb I rebased locally and resolved the issue.

FWIW, this article just ran through JavaScript Weekly, and I feel it's highly relevant. I agree with most of what that article states, but it's purely an opinion, and although I happen to agree with most of it, I'm very against making it the default.

Also, not all languages have default imports/exports (in my experience, most with module systems built-in don't).

coveralls · 2017-11-27T04:36:26Z

Coverage decreased (-0.02%) to 96.068% when pulling 862cdcf on isiahmeadows:no-default-import into 9db95b3 on benmosher:master.

ljharb · 2017-11-27T05:46:10Z

I've seen it - that article is full of "arguments" supporting named exports that apply identically to default exports; it doesn't make a convincing case at all.

Most languages also don't have a package manager that allows conflicting nested dependencies, but luckily JS isn't constrained by that :-)

ljharb · 2017-11-27T05:46:35Z

README.md

 * Prefer named exports to be grouped together in a single export declaration ([`group-exports`])
+* Forbid default exports ([`no-default-export`])


this rebase seems to have left this item here twice

ljharb · 2017-11-27T05:47:45Z

src/rules/no-default-export.js

+
+      ExportNamedDeclaration: function (node) {
+        for (var i = 0; i < node.specifiers.length; i++) {
+          var specifier = node.specifiers[i]


this should use node.specifiers.forEach instead of a loop (which will allow both var instances to be removed)

ljharb · 2017-11-27T05:48:32Z

tests/src/rules/no-default-export.js

+    test({
+      code: 'export { a, b } from "foo.js"',
+      parser: 'babel-eslint',
+      }),


the indentation here is still off; the } should match the indentation level of the line that ends with }

ljharb

whoops, clicked "approve" by accident; there's still some issues left to resolve

dead-claudia · 2017-11-27T08:02:31Z

@ljharb All fixed (plus some other indentation oddities).

coveralls · 2017-11-27T08:17:17Z

Coverage decreased (-0.02%) to 96.066% when pulling 212555f on isiahmeadows:no-default-import into 9db95b3 on benmosher:master.

ljharb

LGTM, pending the last comments

ljharb · 2017-11-29T07:10:00Z

src/rules/no-default-export.js

+      return {}
+    }
+
+    var message = 'Prefer named exports.'


ljharb · 2017-11-29T07:10:07Z

docs/rules/no-default-export.md

@@ -0,0 +1,63 @@
+# no-default-export


dead-claudia · 2017-11-29T07:54:01Z

@ljharb I added a new commit to address your other feedback (that I somehow missed), and I also fixed a bug in the process (with the proposed export foo from "bar.js", which exports a named binding, not a default one).

coveralls · 2017-11-29T08:28:59Z

Coverage decreased (-0.02%) to 96.07% when pulling 9a1450e on isiahmeadows:no-default-import into 9f42bed on benmosher:master.

coveralls · 2018-01-04T08:14:42Z

Coverage decreased (-0.02%) to 96.185% when pulling acf9633 on isiahmeadows:no-default-import into 2aef76e on benmosher:master.

dead-claudia · 2018-01-05T05:25:00Z

Ping, @ljharb / @benmosher?

ljharb · 2018-01-05T06:52:45Z

@isiahmeadows I rebased this branch for you. It's still waiting on reviews from other collabs though.

dead-claudia · 2018-01-16T21:33:55Z

Ping @benmosher / @jfmengels?

dead-claudia · 2018-02-05T17:37:18Z

Ping? @benmosher / @jfmengels

sergeysova · 2018-02-18T16:22:30Z

Any progress here?

benmosher

LGTM 👍

sergeysova · 2018-02-19T16:00:23Z

Add to readme?

dead-claudia · 2018-02-19T16:26:51Z

@sergeysova #1026

Add no-default-export + docs/tests

acf9633

ljharb mentioned this pull request Oct 16, 2017

Rule to prohibit default exports #889

Closed

j-f1 suggested changes Oct 16, 2017

View reviewed changes

dead-claudia force-pushed the no-default-import branch from 79b270b to 5f9e0ce Compare October 17, 2017 01:03

j-f1 reviewed Nov 26, 2017

View reviewed changes

ljharb requested changes Nov 26, 2017

View reviewed changes

ljharb approved these changes Nov 27, 2017

View reviewed changes

ljharb requested changes Nov 27, 2017

View reviewed changes

ljharb reviewed Nov 29, 2017

View reviewed changes

ljharb approved these changes Nov 29, 2017

View reviewed changes

ljharb requested review from benmosher and jfmengels November 29, 2017 07:46

ljharb force-pushed the no-default-import branch from 9a1450e to acf9633 Compare January 4, 2018 07:43

j-f1 mentioned this pull request Jan 9, 2018

Proposal no-default-export #999

Closed

benmosher approved these changes Feb 19, 2018

View reviewed changes

benmosher merged commit 5b0777d into import-js:master Feb 19, 2018

dead-claudia deleted the no-default-import branch February 19, 2018 15:48

dead-claudia mentioned this pull request Feb 19, 2018

Cut new version #976

Closed

dead-claudia mentioned this pull request Feb 19, 2018

Missed a link #1026

Merged

renovate bot mentioned this pull request Feb 22, 2018

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

Merged

dependabot-preview bot mentioned this pull request Mar 21, 2018

Bump eslint-plugin-import from 2.8.0 to 2.9.0 Yproximite/better-vue-input-tag#39

Closed

renovate bot mentioned this pull request Mar 22, 2018

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

Merged

		* Prefer named exports to be grouped together in a single export declaration ([`group-exports`])
		* Forbid default exports ([`no-default-export`])

Add no-default-export + docs/tests #936

Add no-default-export + docs/tests #936

Conversation

dead-claudia commented Oct 1, 2017 • edited Loading

dead-claudia commented Oct 9, 2017

j-f1 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

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 Oct 17, 2017 • edited Loading

dead-claudia commented Nov 26, 2017

ljharb commented Nov 26, 2017

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

dead-claudia commented Nov 27, 2017

coveralls commented Nov 27, 2017 • edited Loading

ljharb commented Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

dead-claudia commented Nov 27, 2017

coveralls commented Nov 27, 2017 • edited Loading

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

dead-claudia commented Nov 29, 2017

coveralls commented Nov 29, 2017 • edited Loading

coveralls commented Jan 4, 2018 • edited Loading

dead-claudia commented Jan 5, 2018 • edited by ljharb Loading

ljharb commented Jan 5, 2018

dead-claudia commented Jan 16, 2018

dead-claudia commented Feb 5, 2018

sergeysova commented Feb 18, 2018

benmosher left a comment

Choose a reason for hiding this comment

sergeysova commented Feb 19, 2018

dead-claudia commented Feb 19, 2018

Add `no-default-export` + docs/tests #936

Add `no-default-export` + docs/tests #936

dead-claudia commented Oct 1, 2017 •

edited

Loading

coveralls commented Oct 17, 2017 •

edited

Loading

coveralls commented Nov 27, 2017 •

edited

Loading

ljharb commented Nov 27, 2017 •

edited

Loading

coveralls commented Nov 27, 2017 •

edited

Loading

coveralls commented Nov 29, 2017 •

edited

Loading

coveralls commented Jan 4, 2018 •

edited

Loading

dead-claudia commented Jan 5, 2018 •

edited by ljharb

Loading