Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 31, 2021

Conversation

ttmarek
Copy link
Contributor

@ttmarek ttmarek commented Apr 14, 2017

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

@coveralls
Copy link

coveralls commented Apr 14, 2017

Coverage Status

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

ljharb
ljharb previously requested changes Apr 14, 2017
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ttmarek
Copy link
Contributor Author

ttmarek commented Apr 14, 2017

@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
Copy link
Member

ljharb commented Apr 14, 2017

Yes, exactly

import readPkgUp from 'read-pkg-up'

function getEntryPoint(context) {
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, let me take a looksee

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

Coverage Status

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

@coveralls
Copy link

coveralls commented Apr 14, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Apr 15, 2017

Coverage Status

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good! Needs docs ofc.

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@coveralls
Copy link

coveralls commented Apr 16, 2017

Coverage Status

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

json needs double quotes

module.exports = foo;

/* in some-file.js */
/* eslint import/no-import-module-exports: ["error", {"exceptions": ['**/*/some-file.js']}] */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this test removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

coveralls commented Apr 16, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Apr 16, 2017

Coverage Status

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

@ljharb ljharb dismissed their stale review April 16, 2017 02:48

LGTM; deferring to other collabs

@ttmarek
Copy link
Contributor Author

ttmarek commented Apr 25, 2017

@benmosher @jfmengels any thoughts?

@ljharb ljharb marked this as a duplicate of #902 Jul 25, 2017
@ttmarek
Copy link
Contributor Author

ttmarek commented Nov 14, 2017

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

@ljharb ljharb force-pushed the no-import-module-exports branch 2 times, most recently from 6e8f406 to 86bbf94 Compare January 31, 2021 21:43
@ljharb ljharb force-pushed the no-import-module-exports branch 2 times, most recently from 1b407b9 to a45661b Compare January 31, 2021 21:57
@ljharb ljharb changed the title Add no-import-module-exports rule [New] Add no-import-module-exports rule Jan 31, 2021
@ljharb ljharb merged commit a45661b into import-js:master Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

add no-import-module-exports
3 participants