Add `no-anonymous-default-export` rule by duncanbeevers · Pull Request #712 · import-js/eslint-plugin-import

duncanbeevers · 2017-01-05T19:25:36Z

Forbids exporting anonymous functions, literals, class expressions, or arrow functions as module default.

This is intended to improve the grepability of codebases by encouraging the re-use of identifiers at their declaration site and at their import sites.

Rule Details

Fail

export default []

export default () => {}

export default class {}

export default function () {}

export default 123

export default {}

Pass

const foo = 123
export default foo

export default class MyClass() {}

export default function foo() {}

/* eslint import/no-anonymous-default-export: [2, {"allowArray": true}] */
export default []

/* eslint import/no-anonymous-default-export: [2, {"allowArrowFunction": true}] */
export default () => {}

/* eslint import/no-anonymous-default-export: [2, {"allowAnonymousClass": true}] */
export default class {}

/* eslint import/no-anonymous-default-export: [2, {"allowAnonymousFunction": true}] */
export default function () {}

/* eslint import/no-anonymous-default-export: [2, {"allowLiteral": true}] */
export default 123

/* eslint import/no-anonymous-default-export: [2, {"allowObject": true}] */
export default {}

coveralls · 2017-01-05T20:06:51Z

Coverage decreased (-0.02%) to 94.844% when pulling 3b25d5b on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

coveralls · 2017-01-05T20:18:31Z

Coverage decreased (-0.02%) to 94.844% when pulling 6a47f2d on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

coveralls · 2017-01-05T21:37:56Z

Coverage increased (+0.03%) to 94.895% when pulling 949ac38 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

ljharb · 2017-01-05T22:17:17Z

Should this then warn if you default-export an arrow function? what about an anonymous class expression?

Why are literals forbidden? The identifier name of their import would usually match the filename, by convention.

duncanbeevers · 2017-01-06T03:10:29Z

The idea is to improve grepability of the codebase. When grepping around for an identifier, filenames aren't matched on, only the contents of the file.

I agree that it should warn if you default-export an arrow function or an anonymous class expression. I'll add specs for those cases.

duncanbeevers · 2017-01-06T03:34:31Z

@ljharb Now forbids default exports of anonymous class expressions and arrow functions. Added functionality, specs and documentation to cover these cases.

coveralls · 2017-01-06T03:38:50Z

Coverage increased (+0.05%) to 94.912% when pulling 72d4d31 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

coveralls · 2017-01-06T03:43:51Z

Coverage increased (+0.05%) to 94.912% when pulling 74af3e3 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

coveralls · 2017-01-06T03:45:26Z

Coverage increased (+0.05%) to 94.912% when pulling 74af3e3 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

ljharb

Currently, this checks for primitive literals, object literals, anon classes, anon functions, and arrows. What would you think about an option for each of these items?

My personal use cases would only enable the functions and classes, but object literals in particular I'd need to be able to turn off.

duncanbeevers · 2017-01-06T07:59:49Z

@ljharb Each forbidden unnamed default export type can now be selectively allowed with an option.

coveralls · 2017-01-06T08:01:10Z

Coverage increased (+0.07%) to 94.93% when pulling 5a8d6ef on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

ljharb

Looking much better, thanks!

Couple comments, and could we add at least one example or note with multiple options passed?

ljharb · 2017-01-06T08:01:30Z

+
+  create: function (context) {
+
+    const forbid = (option) => !~context.options.indexOf('allow-' + option)


let's use > -1 here, !~ is a bit too clever ;-)

ljharb · 2017-01-06T08:01:49Z

+ * @author Duncan Beevers
+ */
+
+module.exports = {


please add a schema to the rule, so that invalid options are rejected.

coveralls · 2017-01-06T08:41:34Z

Coverage increased (+0.07%) to 94.93% when pulling 7c07aa0 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

duncanbeevers · 2017-01-06T10:14:53Z

@ljharb

Added specs demonstrating use of multiple options
Added schema
Removed ~+indexOf gymnastics

coveralls · 2017-01-06T10:22:06Z

Coverage increased (+0.08%) to 94.941% when pulling 818bee2 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

jfmengels

Hi @duncanbeevers! Thanks for the interesting rule and making a PR for it :)

I have written some ways this could be simplified, feel free to disagree if you feel I'm mistaken somewhere.

jfmengels · 2017-01-06T23:51:37Z

+      // Allow forbidden types with multiple options
+      test({ code: 'export default 123', options: ['allow-literal', 'allow-object-expression'] }),
+      test({ code: 'export default {}', options: ['allow-literal', 'allow-object-expression'] }),
+


Could you add few valid tests for other kind of exports (named exports, export * from 'foo').
They are properly ignored at the moment, but it's mostly for documentation's sake.
(Might just be me, but I like to add additional tests slightly out of bounds of what is being treated to prevent surprises)

Added a few specs for a couple of flavors of exports and labeled the section

// Sanity check unrelated export syntaxes

jfmengels · 2017-01-06T23:53:58Z

+
+ruleTester.run('no-anonymous-default-export', rule, {
+    valid: [
+      // Named exports are valid


This is a confusing comment, as named exports are a thing of their own

export const foo = 2;

Instead, "not anonymous", "with a name"? Or whatever you feel like naming it.

Updated this comment to

// Exports with identifiers are valid

jfmengels · 2017-01-06T23:55:12Z

+        const type = node.declaration.type
+        const noID = !node.declaration.id
+
+        if (type === 'Literal' && forbid(MAP[type])) {


You can simplify the conditions by having this pretty much at the start of the function:

if (!forbid(MAP[type])) { return; }

jfmengels · 2017-01-07T00:00:06Z

+        if (type === 'FunctionDeclaration' && noID && forbid(MAP[type])) {
+          context.report({
+            node: node,
+            message: 'Unexpected default export of anonymous function',


I'm not a native speaker, but doesn't of an anonymous function sound better?

I've added articles are all the default export types for consistency.

a literal an array an object

etc...

jfmengels · 2017-01-07T00:04:41Z

+          return
+        }
+
+        if (type === 'ObjectExpression' && forbid(MAP[type])) {


I think you can make all this less repetitive:

const types = { // or whatever you wish to name it ObjectExpression: { validation: () => true, description: 'object expression', }, FunctionDeclaration: { validation: id => !id, description: 'anonymous function', }, // ... } // .... 'ExportDefaultDeclaration': (node) => { if (forbid(MAP[type]) && types[type] && types[type].validation(node.declaration.id)) { context.report({ node: node, message: 'Unexpected default export of ' + types[type].description, }) } }

The architecture of the rule now roughly matches this suggested layout.

jfmengels · 2017-01-07T00:15:24Z

@@ -0,0 +1,51 @@
+# no-anonymous-default-export
+
+Reports if a module's default export is unnamed. This includes several types of unnamed data types; literals, object expressions, anonymous functions, arrow functions, and anonymous class declarations.


I would also enforce this on arrays, which I for instance sometimes use to create a fixtures file.
Also, I think it would make sense to also handle template literals if we intend to handle pretty much any value ?

Agreed - it should handle arrays, but template literals i'd expect to be covered under "literals".

Yes, I was thinking the same thing, no real reason to handle them separately from normal strings and numbers.

Added enforcement for arrays, and a spec for the handling of template literals.

jfmengels · 2017-01-07T00:15:42Z

+module.exports = {
+  meta: {
+    schema: {
+      type: 'array',


I think the options should better be passed as an object, with a true/false values to each key. I found that having one object with settings instead of multiple values in a given order makes it much more extendable.

My gut feeling is to make it so that a field set to true would make it forbidden, and false would make it ignored, and that you'd have it true by default.

/*eslint import/no-anonymous-default-export: [2, "allow-arrow-function-expression"]*/ --> /*eslint import/no-anonymous-default-export: [2, {ArrowFunctionExpression: false}]*/

The schema would look something like this (IIRC) (adapted from what is done in https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-extraneous-dependencies.js)

schema: [ { 'type': 'object', 'properties': { 'Literal': { 'type': 'boolean' }, 'ObjectExpression': { 'type': 'boolean' }, 'FunctionDeclaration': { 'type': 'boolean' }, 'ArrowFunctionExpression': { 'type': 'boolean' }, 'ClassDeclaration': { 'type': 'boolean' }, }, 'additionalProperties': false, }, ],

And you'd get options like

create: function (context) { const options = context.options[0] || {} }

I would be fine with simplifying the name of the properties (like ArrowFunctionExpression --> ArrowFunction), you'd just need to account for the difference in the code.

Options are now handled as a single object. Details are spelled out in the rule description file.

The names are explicit, I like it 👍

coveralls · 2017-01-07T06:59:56Z

Coverage increased (+0.06%) to 94.927% when pulling 2a12e67 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

jfmengels · 2017-01-07T11:18:49Z

+import { RuleTester } from 'eslint'
+
+var ruleTester = new RuleTester()
+var rule = require('rules/no-anonymous-default-export')


var -> const

jfmengels · 2017-01-07T11:23:04Z

+    ],
+
+    invalid: [
+      test({ code: 'export default []', errors: [{ message: 'Unexpected default export of an array' }] }),


I think the error messages are confusing.
Unexpected default export of an anonymous class/function are fine, because they mention that what is wrong is the anonymousness.

For the others, it feels like this rule says that arrays/literals/objects are not allowed to be the default export for some reason.
I think it would be clearer if we recommended giving it a name or assigning it to a variable.

The error messages and schema description strings have all been made explicit. The messages about anonymous functions and classes remain the same, while the rest of the messages are updated to use a more active voice.

'Assign literal to a variable before exporting as module default'

jfmengels · 2017-01-07T11:23:50Z

+      test({ code: 'export default []', errors: [{ message: 'Unexpected default export of an array' }] }),
+      test({ code: 'export default () => {}', errors: [{ message: 'Unexpected default export of an arrow function' }] }),
+      test({ code: 'export default class {}', errors: [{ message: 'Unexpected default export of an anonymous class' }] }),
+      test({ code: 'export default 123', errors: [{ message: 'Unexpected default export of a literal' }] }),


Could you add a test with a string and one with a template literal ?

I added tests with string and template literal cases to both the success and failure corpuses, and found that template literals were not yet forbidden.

I corrected this by adding a TemplateLiteral clause to the defs structure whose value is intentionally identical to Literal's

jfmengels · 2017-01-07T11:27:51Z

+  map((key) => defs[key]).
+  reduce((acc, def) => {
+    acc[def.option] = {
+      description: 'If `false`, will not report default export of ' + def.message,


This sounds like the opposite of what the name suggests.
From what I understand, if allowLiteral is set to true, then Literals will be reported. 🤔

This should be reversed and the default value should be false.

Yes, this fell out of date as I reworked the implementation. Corrected.

jfmengels · 2017-01-07T11:29:27Z

+const defaultOptions = Object.keys(defs).
+  map((key) => defs[key]).
+  reduce((acc, def) => {
+    acc[def.options] = false


def.option ? 🤔

This defaultOptions object doesn't strictly need to be constructed. I created it (poorly) for parity with the documentation. Since it doesn't affect behavior, I'm rewinding my decision here and removing this.

jfmengels

LGTM, thanks for all your work @duncanbeevers! :)

I'm letting the others some time to have a final look, and will merge soon if they don't.

coveralls · 2017-01-07T17:36:13Z

Coverage increased (+0.05%) to 94.91% when pulling ad9e908 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

coveralls · 2017-01-07T17:36:13Z

Coverage increased (+0.05%) to 94.91% when pulling ad9e908 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

coveralls · 2017-01-07T17:36:13Z

Coverage increased (+0.05%) to 94.91% when pulling ad9e908 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

coveralls · 2017-01-07T17:36:13Z

Coverage increased (+0.05%) to 94.91% when pulling ad9e908 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

duncanbeevers · 2017-01-07T18:14:08Z

I added an example case to the documentation of exporting an identified rather than anonymous class.

coveralls · 2017-01-07T18:19:15Z

Coverage increased (+0.05%) to 94.91% when pulling 2a9a7a1 on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

duncanbeevers · 2017-01-16T16:18:58Z

@benmosher Let me know if there's anything I can do to shepherd this along.

jfmengels · 2017-01-21T13:53:42Z

Thanks a lot for this @duncanbeevers! (and sorry for the delay in merging this)

coveralls · 2017-01-21T13:55:15Z

Coverage decreased (-1.3%) to 93.609% when pulling ba3277c on duncanbeevers:no-anonymous-default-export into c975742 on benmosher:master.

duncanbeevers force-pushed the no-anonymous-default-export branch from 3b25d5b to 6a47f2d Compare January 5, 2017 19:34

duncanbeevers force-pushed the no-anonymous-default-export branch from 6a47f2d to 949ac38 Compare January 5, 2017 20:53

duncanbeevers force-pushed the no-anonymous-default-export branch from 949ac38 to 72d4d31 Compare January 6, 2017 03:32

duncanbeevers force-pushed the no-anonymous-default-export branch 2 times, most recently from c7efe8e to 74af3e3 Compare January 6, 2017 03:37

ljharb reviewed Jan 6, 2017

View reviewed changes

ljharb added evaluating question rule proposal semver-minor labels Jan 6, 2017

duncanbeevers force-pushed the no-anonymous-default-export branch from 74af3e3 to 5a8d6ef Compare January 6, 2017 07:56

ljharb requested changes Jan 6, 2017

View reviewed changes

duncanbeevers force-pushed the no-anonymous-default-export branch from 5a8d6ef to 7c07aa0 Compare January 6, 2017 08:36

duncanbeevers force-pushed the no-anonymous-default-export branch from 7c07aa0 to 818bee2 Compare January 6, 2017 10:13

duncanbeevers force-pushed the no-anonymous-default-export branch 2 times, most recently from babb7cd to b1dafb3 Compare January 6, 2017 10:24

ljharb removed evaluating question labels Jan 6, 2017

ljharb requested review from benmosher and jfmengels January 6, 2017 16:56

jfmengels requested changes Jan 7, 2017

View reviewed changes

duncanbeevers force-pushed the no-anonymous-default-export branch from b1dafb3 to 2a12e67 Compare January 7, 2017 06:54

jfmengels requested changes Jan 7, 2017

View reviewed changes

duncanbeevers force-pushed the no-anonymous-default-export branch from 2a12e67 to ad9e908 Compare January 7, 2017 17:29

jfmengels approved these changes Jan 7, 2017

View reviewed changes

Add no-anonymous-default-export rule

2a9a7a1

duncanbeevers force-pushed the no-anonymous-default-export branch from ad9e908 to 2a9a7a1 Compare January 7, 2017 18:13

ljharb approved these changes Jan 10, 2017

View reviewed changes

Add link to PR

ba3277c

jfmengels merged commit f43cf95 into import-js:master Jan 21, 2017

duncanbeevers deleted the no-anonymous-default-export branch January 21, 2017 17:50

renovate-bot mentioned this pull request May 24, 2017

Update dependency eslint-plugin-import to version 2.3.0 renovatebot/renovate#217

Merged

ljharb mentioned this pull request Jan 4, 2018

Rule Proposal: export-style #637

Closed

This was referenced Apr 15, 2018

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

Merged

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

Merged

Gudahtt mentioned this pull request Apr 6, 2022

Upgrade Jest and use TS for Jest config MetaMask/metamask-module-template#97

Merged


		create: function (context) {

		const forbid = (option) => !~context.options.indexOf('allow-' + option)

		@@ -0,0 +1,51 @@
		# no-anonymous-default-export

		Reports if a module's default export is unnamed. This includes several types of unnamed data types; literals, object expressions, anonymous functions, arrow functions, and anonymous class declarations.

Uh oh!

Conversation

duncanbeevers commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rule Details

Fail

Pass

Uh oh!

coveralls commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Jan 5, 2017

Uh oh!

duncanbeevers commented Jan 6, 2017

Uh oh!

duncanbeevers commented Jan 6, 2017

Uh oh!

coveralls commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

duncanbeevers commented Jan 6, 2017

Uh oh!

coveralls commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duncanbeevers commented Jan 6, 2017

Uh oh!

coveralls commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfmengels left a comment

Choose a reason for hiding this comment

Uh oh!

jfmengels Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfmengels Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncanbeevers commented Jan 5, 2017 •

edited

Loading

coveralls commented Jan 5, 2017 •

edited

Loading

coveralls commented Jan 5, 2017 •

edited

Loading

coveralls commented Jan 5, 2017 •

edited

Loading

coveralls commented Jan 6, 2017 •

edited

Loading

coveralls commented Jan 6, 2017 •

edited

Loading

coveralls commented Jan 6, 2017 •

edited

Loading

coveralls commented Jan 6, 2017 •

edited

Loading

coveralls commented Jan 6, 2017 •

edited

Loading

coveralls commented Jan 6, 2017 •

edited

Loading

jfmengels Jan 6, 2017 •

edited

Loading

jfmengels Jan 6, 2017 •

edited

Loading

jfmengels Jan 7, 2017 •

edited

Loading

coveralls commented Jan 7, 2017 •

edited

Loading