[New] Add `no-import-module-exports` rule #804

ttmarek · 2017-04-14T21:21:58Z

Fixes #760
Still a work in progress.
Updates to the changelog and docs to follow.

… with CommonJS exports Fixes import-js#760

coveralls · 2017-04-14T21:28:04Z

Coverage increased (+0.02%) to 94.936% when pulling 6f19984 on ttmarek:no-import-module-exports into 106740f on benmosher:master.

ljharb

Could we also handle assignments to the exports object? Like exports.foo = bar

ttmarek · 2017-04-14T23:00:17Z

@ljharb
sure...just to be clear, you're thinking we should ensure that the following is invalid:

    test({
      code: `
        import thing from 'other-thing'
        exports.foo = bar
      `,
      errors: [error],
    })

and the following would be valid:

    test({
      code: `
        const thing = require('thing')
        exports.foo = bar
      `,
    }),

ljharb · 2017-04-14T23:05:48Z

Yes, exactly

ljharb · 2017-04-14T23:10:12Z

src/rules/no-import-module-exports.js

+import readPkgUp from 'read-pkg-up'
+
+function getEntryPoint(context) {
+  try {


I'm wondering if it'd be easier to just use require.resolve(pathToPackageJSON) here, which reads "main" for you?

hmm...I'm not sure I follow.
From what I understand require.resolve gives you the absolute path to a given module.
So if I wanted the path to minimatch's package.json I could do:

require.resolve('minimatch/package.json')

But I can't use it to get an absolute path to the current module's package.json...Or can I?

By the way, I borrowed this approach from no-extraneous-dependencies. If we come up with a better solution, it might not be a bad idea to implement it there as well.

If you require.resolve a dir path that contains a package.json, it gives you the path to the "main"

I'm suggesting finding the package.json first, and then resolving that dir. Not sure if it will work or not here.

I see what you mean, let me take a looksee

The require.resolve approach seems to work well. The only thing to take into consideration is the use of read-pkg-up for finding the path to the nearest package.json. Using read-pkg-up the implementation would look something like:

import path from 'path' import readPkgUp from 'read-pkg-up' function getEntryPoint(context) { const pkg = readPkgUp.sync({ cwd: context.getFilename() }) return require.resolve(path.dirname(pkg.path)) }

But since all we need is the path to the nearest package.json (we don't actually need to parse it) we could use the pkg-up lib instead. In which case the implementation would look like:

import path from 'path' import pkgUp from 'pkg-up' function getEntryPoint(context) { const pkgPath = pkgUp.sync(context.getFilename()) return require.resolve(path.dirname(pkgPath)) }

So, we can:
A) Stick to the current implementation where we parse the package.json, and create the path to "main" ourselves.
B) Use read-pkg-up to find the path to the nearest package.json, then use require.resolve to get the path to main.
C) Install and use pkg-up to find the path to the nearest package.json, then use require.resolve to get the path to main.

I prefer B and C over A....and lean towards C, but I'm not sure what the cost would be of adding another dependency. What do you think?

pkg-up is already a dep of read-pkg-up, and the cost of adding dependencies to a dev dependency is virtually zero. I'd say C.

However, I just realized my comment on the original issue does also add that it should probably support jsnext:main, and for that, you'd need A or B.

I'd still lean towards C for the primary case.

C it is. Here's the diff.

Those using jsnext:main can always add their second entry to the exceptions in the rule options. I will add something to that effect in the docs.

coveralls · 2017-04-14T23:13:31Z

Coverage decreased (-1.3%) to 93.656% when pulling d8d0bff on ttmarek:no-import-module-exports into 106740f on benmosher:master.

coveralls · 2017-04-14T23:13:31Z

Coverage increased (+0.02%) to 94.936% when pulling d8d0bff on ttmarek:no-import-module-exports into 106740f on benmosher:master.

coveralls · 2017-04-15T19:12:52Z

Coverage increased (+0.07%) to 94.986% when pulling e2a8ccb on ttmarek:no-import-module-exports into 106740f on benmosher:master.

ljharb

Seems good! Needs docs ofc.

ljharb · 2017-04-15T22:52:59Z

src/rules/no-import-module-exports.js

+      const isIdentifier = node.object.type === 'Identifier'
+      const hasKeywords = (/^(module|exports)$/).test(node.object.name)
+      const isException = options.exceptions
+        ? options.exceptions.some(glob => minimatch(fileName, glob))


this could be options.exceptions && options.exceptions.some(…), but this is fine too :-)

coveralls · 2017-04-16T00:02:35Z

Coverage increased (+0.07%) to 94.986% when pulling 2894b49 on ttmarek:no-import-module-exports into 106740f on benmosher:master.

ljharb · 2017-04-16T00:06:42Z

docs/rules/no-import-module-exports.md

+
+```json
+"import/no-import-module-exports": ["error", {
+    "exceptions": ['**/*/some-file.js']


json needs double quotes

ljharb · 2017-04-16T00:17:28Z

docs/rules/no-import-module-exports.md

+module.exports = foo;
+
+/* in some-file.js */
+/* eslint import/no-import-module-exports: ["error", {"exceptions": ['**/*/some-file.js']}] */


these should also probably be double quotes, but also, the */ actually does end the comment, so this should probably be a // comment :-)

ljharb · 2017-04-16T00:17:43Z

tests/src/rules/no-import-module-exports.js

@@ -93,16 +93,6 @@ ruleTester.run('no-import-module-exports', rule, {
        import foo from 'path';
        module.exports = foo;
      `,
-      // When the file does not match the entry point defined in package.json
-      // See tests/files/package.json


why was this test removed?

Ahh....the four tests above this one are already testing that the rule applies for files that aren't "main". By default the test util sets the default file to eslint-plugin-import/tests/files/foo.js. Which isn't equal to the "main" I set up in eslint-plugin-import/tests/files/package.json. It seemed a bit redundant to me...but I can add it back if you prefer.

coveralls · 2017-04-16T02:09:26Z

Coverage increased (+0.07%) to 94.986% when pulling e2f13a7 on ttmarek:no-import-module-exports into 106740f on benmosher:master.

coveralls · 2017-04-16T02:15:00Z

Coverage increased (+5.3%) to 76.063% when pulling a45661b on ttmarek:no-import-module-exports into fe51583 on benmosher:master.

LGTM; deferring to other collabs

ttmarek · 2017-04-25T15:21:17Z

@benmosher @jfmengels any thoughts?

ttmarek · 2017-11-14T19:58:51Z

🎵 all I want for christmaaas 🎵... is for this to be merged. 🎅

[New] add no-import-module-exports rule: report import declarations…

a45661b

… with CommonJS exports Fixes import-js#760

ljharb added the semver-minor label Apr 14, 2017

ljharb previously requested changes Apr 14, 2017

View reviewed changes

ljharb reviewed Apr 14, 2017

View reviewed changes

ljharb reviewed Apr 15, 2017

View reviewed changes

ljharb reviewed Apr 16, 2017

View reviewed changes

ljharb requested review from benmosher and jfmengels April 16, 2017 02:48

ljharb marked this as a duplicate of #902 Jul 25, 2017

ljharb mentioned this pull request Jul 25, 2017

Add no-commonjs-exports-with-import rule #902

Closed

ljharb force-pushed the no-import-module-exports branch 2 times, most recently from 6e8f406 to 86bbf94 Compare January 31, 2021 21:43

ljharb approved these changes Jan 31, 2021

View reviewed changes

ljharb force-pushed the no-import-module-exports branch 2 times, most recently from 1b407b9 to a45661b Compare January 31, 2021 21:57

ljharb changed the title ~~Add no-import-module-exports rule~~ [New] Add no-import-module-exports rule Jan 31, 2021

ljharb merged commit a45661b into import-js:master Jan 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New] Add `no-import-module-exports` rule #804

[New] Add `no-import-module-exports` rule #804

ttmarek commented Apr 14, 2017

coveralls commented Apr 14, 2017 •

edited

ljharb left a comment

ttmarek commented Apr 14, 2017

ljharb commented Apr 14, 2017

ljharb Apr 14, 2017

ttmarek Apr 14, 2017

ljharb Apr 15, 2017

ljharb Apr 15, 2017

ttmarek Apr 15, 2017

ttmarek Apr 15, 2017

ljharb Apr 15, 2017

ttmarek Apr 15, 2017

coveralls commented Apr 14, 2017

coveralls commented Apr 14, 2017 •

edited

coveralls commented Apr 15, 2017 •

edited

ljharb left a comment

ljharb Apr 15, 2017

coveralls commented Apr 16, 2017 •

edited

ljharb Apr 16, 2017

ljharb Apr 16, 2017

ljharb Apr 16, 2017

ttmarek Apr 16, 2017

coveralls commented Apr 16, 2017 •

edited

coveralls commented Apr 16, 2017 •

edited

ttmarek commented Apr 25, 2017

ttmarek commented Nov 14, 2017

[New] Add no-import-module-exports rule #804

[New] Add no-import-module-exports rule #804

Conversation

ttmarek commented Apr 14, 2017

coveralls commented Apr 14, 2017 • edited

ljharb left a comment

Choose a reason for hiding this comment

ttmarek commented Apr 14, 2017

ljharb commented Apr 14, 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

coveralls commented Apr 14, 2017

coveralls commented Apr 14, 2017 • edited

coveralls commented Apr 15, 2017 • edited

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 16, 2017 • edited

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 Apr 16, 2017 • edited

coveralls commented Apr 16, 2017 • edited

ttmarek commented Apr 25, 2017

ttmarek commented Nov 14, 2017

[New] Add `no-import-module-exports` rule #804

[New] Add `no-import-module-exports` rule #804

coveralls commented Apr 14, 2017 •

edited

coveralls commented Apr 14, 2017 •

edited

coveralls commented Apr 15, 2017 •

edited

coveralls commented Apr 16, 2017 •

edited

coveralls commented Apr 16, 2017 •

edited

coveralls commented Apr 16, 2017 •

edited