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 rule `default-import-match-filename` #1476

Open
wants to merge 6 commits into
base: master
from

Conversation

@golopot
Copy link
Contributor

commented Sep 15, 2019

Closes #325 .

  • Import name is considered to matche filename if they are equal after toLowercase, and striping extensions, and striping characters "._-".
  • CommonJs is also checked.
  • Cases like "./", "../", are not now checked.
@coveralls

This comment has been minimized.

Copy link

commented Sep 15, 2019

Coverage Status

Coverage increased (+0.9%) to 96.268% when pulling e4453dd on golopot:default-import-match-filename into 5e143b2 on benmosher:master.

@golopot golopot force-pushed the golopot:default-import-match-filename branch from 86d60cf to dcb5956 Sep 15, 2019
Copy link
Collaborator

left a comment

I think this rule should also support a list of exceptions/aliases; ie, "fooBar" is allowed anywhere "foo" would otherwise be required.

```js
import bar from './foo';
import utilsFoo from '../utils/foo';
import foo from '../foo/index.js';

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 15, 2019

Collaborator

this should not be a failure; the proper name for any foo/index is foo.

This comment has been minimized.

Copy link
@golopot

golopot Sep 17, 2019

Author Contributor

indeed

* @returns {string}
*/
function removeExtension(filename) {
return filename.replace(/\.[^.]+$/, '')

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 15, 2019

Collaborator
Suggested change
return filename.replace(/\.[^.]+$/, '')
return path.basename(filename, path.extname(filename))

with an import path from 'path' at the top

This comment has been minimized.

Copy link
@golopot

golopot Sep 17, 2019

Author Contributor

done

* @returns {string | undefined}
*/
function getFilename(path) {
if (!isLocalModule(path)) return undefined

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 15, 2019

Collaborator

why would we not want this enforced on things from node_modules? import foo from 'package/bar' should be an error.

This comment has been minimized.

Copy link
@golopot

golopot Sep 17, 2019

Author Contributor

done; however there may be some annoying cases because not every packages comply to the convention this rule enforces:

import ReactDomSever from 'react-dom/server';
import somePkg from 'some-pkg/esm';
import packageJson from 'typescript/package.json';

This comment has been minimized.

Copy link
@golopot

golopot Oct 1, 2019

Author Contributor

The gist is that this that rule can be annoying if you cannot control the filename, like when it a file in other peoples' package, which does not adhere to this rule.

function getFilename(path) {
if (!isLocalModule(path)) return undefined
const [, filename] = /\/([^/]*)$/.exec(path)
if (filename === '' || filename === '.' || filename === '..') {

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 15, 2019

Collaborator

with either . or ../, i would expect the import name to be "the current directory name" (which would prefer package.json's name, if there was a package.json in the current directory).

This comment has been minimized.

Copy link
@golopot

golopot Sep 17, 2019

Author Contributor

Done. File IO scares me.

This comment has been minimized.

Copy link
@golopot

golopot Oct 1, 2019

Author Contributor

I think this use case brings lots of complexity (if then else, file IO, and reading package.json) with little benefit. Using require('..') or require('../..') happens rare enough and is an anti pattern IMO. I would like to ignore this case.

And a hairy case: import src from '..';, when .. resolves to src.

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 9, 2019

Collaborator

In no way is it an antipattern; and it happens in every test dir in all my projects.

The recursive case is just as hairy with from '.', and both need to be handled.

@golopot

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

What does the "exception" option looks like?

// When "name" and "path" both globly match an item in whitelist, the error is ignored.
{
  whitelist: [
    {
      name: '*', // any name is allowed
      path: '*/models/*',
    },
    {
      name: 'reactDomSever',
      path: 'react-dom/sever',
    },
  ];
}
@golopot golopot force-pushed the golopot:default-import-match-filename branch from dcb5956 to 4800988 Sep 17, 2019
@golopot golopot force-pushed the golopot:default-import-match-filename branch from 4800988 to 217ec29 Oct 1, 2019

#### `ignorePaths`

Set this option to `['some-dir/', 'bb']` to ignore import statements whose path contains either `some-dir/` or `bb` as a substring.

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 9, 2019

Collaborator

should this support globs, and that should be in the example?

function isAncestorRelativePath(path) {
return (
path.length > 0 &&
path[0] !== '/' &&

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 9, 2019

Collaborator
Suggested change
path[0] !== '/' &&
!path.startsWith('/') &&
*/
function getPackageJsonName(packageJsonPath) {
try {
const packageJsonContent = fs.readFileSync(packageJsonPath).toString()

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 9, 2019

Collaborator
Suggested change
const packageJsonContent = fs.readFileSync(packageJsonPath).toString()
const packageJsonContent = String(fs.readFileSync(packageJsonPath))

always String(x), never x.toString()

* @returns {string | undefined}
*/
function getPackageJsonName(packageJsonPath) {
try {

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 9, 2019

Collaborator

why can't this do a require(packageJsonPath).name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.