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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

golopot
Copy link
Contributor

@golopot golopot 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.

Co-Authored-By: Chiawen Chen <[email protected]>
Co-Authored-By: Armano <[email protected]>
Co-Authored-By: Sharmila <[email protected]>
@coveralls
Copy link

coveralls commented Sep 15, 2019

Coverage Status

Coverage increased (+0.003%) to 97.841% when pulling 678fc02 on golopot:default-import-match-filename into 47f912e on benmosher:master.

@golopot golopot force-pushed the default-import-match-filename branch from 86d60cf to dcb5956 Compare September 15, 2019 16:42
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.

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

@golopot
Copy link
Contributor Author

golopot 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 default-import-match-filename branch from dcb5956 to 4800988 Compare September 17, 2019 22:19
@golopot golopot force-pushed the default-import-match-filename branch from 4800988 to 217ec29 Compare October 1, 2019 06:05

#### `ignorePaths`

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that globs is not a good tool against relative paths.

Copy link
Member

Choose a reason for hiding this comment

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

can you elaborate? it seems like a good tool to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To perform a glob matching a cwd is required, for example:

// Using a hypothetical glob library
glob.match({
  pattern: 'tests/**/*.js'
  target: '/home/john/project-a/tests/a.js'
  cwd: '/home/john/project-a'
})
// true

What should the cwd be in this rule? The first thought is to use the location of eslintrc as cwd, but it is problematic when there is another eslintrc in a subdirectory, or when overrides is used.

Copy link
Member

Choose a reason for hiding this comment

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

the cwd for any eslint rule is always process.cwd(), since you can't be sure where the config file is located.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using process.cwd() taken as the cwd means that the rule will give different result depending on the cwd of the shell, is that fine?

// option for this rule
{
  ignoredPaths: ['tests/**/*.js']
}

project-root$ npx eslint . # will ignore files in tests/ 
project-root/tests$ npx eslint . # will **not** ignore files in tests/

Copy link
Member

Choose a reason for hiding this comment

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

yes, that seems fine to me.

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

Choose a reason for hiding this comment

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

Suggested change
path[0] !== '/' &&
!path.startsWith('/') &&

Copy link

Choose a reason for hiding this comment

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

Suggested change
path[0] !== '/' &&
(path[0] !== '/' || path[0] !== '\\') &&

Copy link
Member

Choose a reason for hiding this comment

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

On a windows machine, an absolute path would start with c://, no?

Copy link

@armano2 armano2 Feb 17, 2020

Choose a reason for hiding this comment

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

yes, you can use C:\ or C:/ and all duplicated slashes and backslashes are reduced than C:// or C://////////// is working,

you can also mix them up: C:/\/\/\/\ but who would do that

V:\> cd C:/\/\/\/\/\/\
C:\>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb
Copy link
Member

ljharb commented Feb 14, 2020

ping @golopot, it'd be great to get this in :-)

@golopot golopot force-pushed the default-import-match-filename branch from 4c48012 to b32abff Compare February 14, 2020 04:43
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.

It might be helpful for users to be able to specify that the binding should be in camelCase, or snake_case, so it won't conflict with the camelCase rule?

Let's add test cases that import from foo_bar as well as fooBar, for both cases.

@golopot
Copy link
Contributor Author

golopot commented Feb 15, 2020

Currently the rule is case-insesative, and underscores are ignored. Variable name foo_BarBAeZ is allowed for file foo_bar_baz.

https://github.com/benmosher/eslint-plugin-import/blob/dfdd7d43a06c1f449788a9921b2973c7f13e0eab/tests/src/rules/default-import-match-filename.js#L50-L53

@ljharb ljharb force-pushed the default-import-match-filename branch 2 times, most recently from d60737d to 830a76b Compare February 17, 2020 08:18
@golopot
Copy link
Contributor Author

golopot commented Feb 29, 2020

I am having trouble in designing the option ignorePatterns. This option is meant to ignore import statements when the import source matches a configured pattern. There are three choices:

  1. (Current implementation) Use the resolved absolute path to match against the patterns. For example, in import a from './foo', the source resolves to absolute path /home/me/my-project/src/foo.js, and that path is used to match against a pattern, say **/foo.js. In this case the pattern **/foo.js matches. This approach has the drawback that only patterns starting with **/ can ever match, and "relative" patterns like src/foo.js or foo.js will never match in general.

  2. Use the directory containing the closest package.json as the "cwd" for glob matching, With this approach, patterns src/foo.js or foo.js are considered to be relative to /home/me/my-project, which is the location of the closest package.json.

  3. Use process.cwd as the "cwd" for glob matching. This approach has the drawback that the lint result depends on the cwd used for running the linter.

@ljharb
Copy link
Member

ljharb commented Mar 10, 2020

Typically, process.cwd() is the current dir - because a rule can't know if its config came from a file (or which one), or from command line config, or an inline comment.

With option 1, however - could that resolved path have process.cwd() stripped off it, and all patterns applied relative to that?

@golopot
Copy link
Contributor Author

golopot commented Mar 10, 2020

With option 1, however - could that resolved path have process.cwd() stripped off it, and all patterns applied relative to that?

It should be doable using the function path.relative(). That sounds like option 3 in #1476 (comment)?

@ljharb
Copy link
Member

ljharb commented Mar 13, 2020

Yes, option 3 sounds right to me.

@stropho
Copy link
Contributor

stropho commented Mar 18, 2020

Looking forward to this rule. Is there any autofix provided? Or not for now ?

@golopot golopot requested a review from ljharb March 20, 2020 03:05
@Pearce-Ropion
Copy link
Contributor

Pearce-Ropion commented May 9, 2020

It would be great to see a little bit more control over how this rule operates in the parserOptions. For example, the ability to make this case sensitive or the ability to whitelist which characters are ignored.

// These would be the defaults
{
    caseSensitive: false,
    allowedChars: [".", "_", "-"],
}

For example:
My repo uses absolute imports and I would like to ensure that the component is imported exactly as the filename is styled. Lets say I have a component called GridColumn:

parserOptions: {
    caseSensitive: true,
    allowedChars: [],
}

pass - import GridColumn from "components/GridColumn";
fail - import gridColumn from "components/GridColumn";
fail - import Gridcolumn from "components/GridColumn";
fail - import gridcolumn from "components/GridColumn";
fail - import Grid-Column from "components/GridColumn";
fail - import Grid_Column from "components/GridColumn";
fail - import GRIDCOLUMN from "components/GridColumn";
etc...

edit: typo

@ljharb
Copy link
Member

ljharb commented Jun 1, 2020

@Pearce-Ropion while the casing should ideally match, remember that not all filesystems are case-sensitive.

@Pearce-Ropion
Copy link
Contributor

@Pearce-Ropion while the casing should ideally match, remember that not all filesystems are case-sensitive.

Of course, thats why I suggested it as an optional setting. My reasoning here that some people have the luxury of knowing what systems their code is being developed on (for example, every engineer at my company is required to use a linux machine). Also, after having a glance at the code, it looks like the isCompatible() fn is doing a string equality check which would make it relatively easy to add these options.

Suggested changes snippet

/**
 * @param {string} filename
 * @param {object} options
 * @returns {string}
 */
function normalizeFilename(filename, options) {
  const allowedCharsRegex = new RegExp(`[${options.allowedChars.map(c => `\${c}`)}]`, 'g');

  if (options.caseSensitive) {
    return filename.replace(allowedCharsRegex, '');
  }

  return filename.replace(allowedCharsRegex, '').toLowerCase()
}

/**
 * Test if local name matches filename.
 * @param {string} localName
 * @param {string} filename
 * @param {object} options
 * @returns {boolean}
 */
function isCompatible(localName, filename, options) {
  const allowedCharsRegex = new RegExp(`[${options.allowedChars.map(c => `\${c}`)}]`, 'g');

  let normalizedLocalName = localName.replace(allowedCharsRegex, '');
  if (!options.caseSensitive) {
    normalizedLocalName = normalizedLocalName.toLowerCase();
  }

  return (
    normalizedLocalName === normalizeFilename(filename, options) ||
    normalizedLocalName === normalizeFilename(removeExtension(filename), options)
  )
}

@granmoe
Copy link

granmoe commented Aug 11, 2020

It will be awesome to have this rule 👍 Let's merge this thing 🚀

@epiqueras
Copy link

#325 (comment)

I think that should be part of this. When would you ever want to enforce this for an import name and not the export name, granted it's an internal module.

@fregante
Copy link
Contributor

Does this need further work or is it done?

@ljharb
Copy link
Member

ljharb commented Jul 20, 2021

It needs further work, as far as I'm aware. I'll mark it as a draft.

@ljharb ljharb marked this pull request as draft July 20, 2021 05:46
@dword-design
Copy link

I'm also working on an NPM package. But is quite some work because you need lots of options for customization. Will write when it's published.

@giladgd
Copy link

giladgd commented Apr 24, 2023

Is there any update on this? It would be great to have this as part of eslint-plugin-import IMO

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.

Proposal: default-import-match-filename