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 :+1:

[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 :+1:

[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 :)