add no-import-module-exports

[kentcdodds]

Due to this issue with webpack tree-shaking, and the fact that some people cannot adopt no-commonjs quite yet, I think this would be really handy. I've written the rule for my own stuff and would be happy to add it here (or retrofit no-commonjs with options to cover this use case).

Here's my rule:

'use strict'

module.exports = {
	meta: {
		docs: {
			description: 'Disallow import statements with module.exports',
			category: 'Best Practices',
			recommended: true,
		},
		fixable: 'code',
		schema: [],
	},
	create(context) {
		const importDeclarations = []
		let hasReported = false
		function report() {
			if (hasReported) {
				return
			}
			hasReported = true
			importDeclarations.forEach(node => {
				context.report({
					node,
					message: 'Cannot use import declarations in modules that export using CommonJS (`module.exports = \'foo\'` or `exports.bar = \'hi\'`)',
				})
			})
		}
		return {
			ImportDeclaration(node) {
				importDeclarations.push(node)
			},
			MemberExpression(node) {
				if (isModuleExports(node)) {
					report()
				}

				function isModuleExports(n) {
					return n.object.type === 'Identifier' && (/^(module|exports)$/).test(n.object.name)
				}
			},
		}
	},
}

And the associated tests:

'use strict'

const { RuleTester } = require('eslint')
const rule = require('./no-import-module-exports')

const parserOptions = { ecmaVersion: 6, sourceType: 'module' }
const error = {
	message: 'Cannot use import declarations in modules that export using CommonJS (`module.exports = \'foo\'` or `exports.bar = \'hi\'`)',
	type: 'ImportDeclaration',
}

const ruleTester = new RuleTester()
ruleTester.run('no-blockless-if', rule, {
	valid: [
		{
			code: `
				const thing = require('thing')
				module.exports = thing
			`,
			parserOptions,
		},
		{
			code: `
				import thing from 'otherthing'
				console.log(thing.module.exports)
			`,
			parserOptions,
		},
		{
			code: `
				import thing from 'other-thing'
				export default thing
			`,
			parserOptions,
		},
	],
	invalid: [
		{
			code: `
				import { stuff } from 'starwars'
				module.exports = thing
			`,
			errors: [error],
			parserOptions,
		},
		{
			code: `
				import thing from 'starwars'
				const baz = module.exports = thing
				console.log(baz)
			`,
			errors: [error],
			parserOptions,
		},
		{
			code: `
				import * as allThings from 'starwars'
				exports.bar = thing
			`,
			errors: [error],
			parserOptions,
		},
	],
})

Let me know what you all think :)

[ljharb]

I like this in most cases, but this seems like it would prevent the very best practice of, in your package's entry point, doing import foo from 'path'; module.exports = foo; so as not to expose consumers to babel's .default (which they should never see).

[kentcdodds]

Sure, doesn't have to be enabled by default 👍

[ljharb]

Right, but it means I wouldn't be able to enable it at all in anything that wasn't a top-level app.

What if it:
a) automatically allowed it in a file that was the "main" or "jsnext:main" (since this plugin supports it) in your package.json, and
b) allowed an "entrypoints" or "exceptions" config array that allowed it in any file/glob pattern matched?

That way by default it allows your entry point to do the right thing (so I can both use and recommend it) and it's configurable if you have multiple entry points.

[kentcdodds]

Sounds solid 👍

[ttmarek]

@kentcdodds which issue with webpack 2 tree shaking are you referring to? I'd like to add a blurb about it in the docs. Also, any other motivating factors for the rule would be welcome.

[kentcdodds]

Whoops! I forgot to link to the issue. It's this one :)