[New] Add `no-import-module-exports` rule by ttmarek · Pull Request #804 · import-js/eslint-plugin-import

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

+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

+      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

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


json needs double quotes

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

+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

      errors: [error],
    }),
-    test({
-      code: `


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

Uh oh!

Conversation

ttmarek commented Apr 14, 2017

Uh oh!

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

ttmarek commented Apr 14, 2017

Uh oh!

ljharb commented Apr 14, 2017

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!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 14, 2017

Uh oh!

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

Uh oh!

Uh oh!

coveralls commented Apr 15, 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!

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

Uh oh!

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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ttmarek commented Apr 25, 2017

Uh oh!

ttmarek commented Nov 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

coveralls commented Apr 14, 2017 •

edited

Loading

coveralls commented Apr 14, 2017 •

edited

Loading

coveralls commented Apr 15, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading