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

Proposal: default-import-match-filename #325

Open
gajus opened this issue May 9, 2016 · 20 comments · May be fixed by #1476
Open

Proposal: default-import-match-filename #325

gajus opened this issue May 9, 2016 · 20 comments · May be fixed by #1476

Comments

@gajus
Copy link
Contributor

gajus commented May 9, 2016

I often see things such as:

import parseDate from './types/date';
import float from './types/parseFloat';

The disparity between the default import name (parseDate) and file name (date) often indicates that one or the other is named inappropriately. In the first case, thats the file name. In the second case, thats the local variable name.

Valid code:

import parseFloat from './types/parseFloat';
import parseDate from './types/parseDate';

Invalid code:

import parseDate from './types/date';
import float from './types/parseFloat';

There is no way to enforce a right name, e.g. user could just as well end up with:

import date from './types/date';
import float from './types/float';

However, I would argue that these issues are easy to notice in the code. Having this rule would prevent lazy developers from using a better variable name in a local file and ignore the need to rename the file and other references.

Furthermore, this enables easier code inspection (because it is easy to recognise what a variable represents when it is being used consistently between many files).

@benmosher
Copy link
Member

Makes sense to me. Probably needs a little more detail/scope to build out.

How would you propose packages be handled? I.E.

import React from 'react'

is idiomatic, but not sure how one would divine this from just the import path.

@gajus
Copy link
Contributor Author

gajus commented May 9, 2016

I would propose to have a separate rule for overseeing module imports and separate rule for importing files, i.e. default-import-match-module and default-import-match-filename. I will leave the question of react case to whoever becomes a proponent of the default-import-match-module rule.

@benmosher
Copy link
Member

Got it, so this rule's scope would be only internal relative imports, as you've described?

@jfmengels
Copy link
Collaborator

jfmengels commented May 9, 2016

We've had a similar discussion on eslint-plugin-xo sindresorhus/eslint-plugin-unicorn#6, though we only spoke about modules (but I think they should probably be considered the same).

I have trouble seeing this as something that can be generalized enough. For example, how would you enforce / generalize the following?

import parseFloat from './parse/float';
import parseDate from './parse/date';
import formatFloat from './format/float';
import formatDate from './format/date';

or

import closeToFoo from './foo';

function foo(...) { ... }

export foo;

cc @jamestalmage @sindresorhus

@benmosher
Copy link
Member

I would think that such a folder layout simply couldn't use the rule that @gajus has described.

It would be workable as he has described if it stays as simple as "default import identifiers should match an imported file's name".

@gajus
Copy link
Contributor Author

gajus commented May 9, 2016

First of all, if you have:

import parseFloat from './parse/float';
import parseDate from './parse/date';
import formatFloat from './format/float';
import formatDate from './format/date';

This is specifically the design flaw that I am describing.

What is the reason you are naming your variables different than the files? In this specific context, it is because variable name makes more sense in the code than simply "float" or "date". Then what is the reason you are naming a file containing "parseFloat" function as "float"? It makes no sense. When considering a file name, file path should have no weight. With this rule, you would be forced to restructure your code as:

import parseFloat from './parse/parseFloat';
import parseDate from './parse/parseDate';
import formatFloat from './format/formatFloat';
import formatDate from './format/formatDate';

(or choose not to use the rule).

@jfmengels
Copy link
Collaborator

When considering file name, file path should have no weight

Hm, I'd like to disagree. I don't see why the file path should have no weight. If I have my float handler in a parse/ folder, then I'm just repeating myself, and I don't see the point. I also often have a folder with an index.js file (like foo/bar/index.js), so that I can just do import bar from './foo/bar'; and "hide" the internal workings in other files. It's not per se in conflict with the rules, just pointing out that I need the file path to infer what foo/bar/index.js does, while being a valid use IMO. This pattern has worked fine for me thus far.
(if you guys feel like I'm taking the conversion somewhere it shouldn't, I'll be happy to discuss this somewhere else).

(or choose not to use the rule)

That is of course an acceptable solution ;) I have no issue with this issue being in the repo at all. I just have a concern that if you generalize this to external modules (it is similar, but I guess that it is done for different purposes here, so one could argue one way or the other IMO), then you'd have issues all over the place for handling it one way or another, have exceptions, etc., and that could be a huge burden on the maintainer.

@gajus
Copy link
Contributor Author

gajus commented May 9, 2016

Hm, I'd like to disagree. I don't see why the file path should have no weight. If I have my float handler in a parse/ folder, then I'm just repeating myself, and I don't see the point. I also often have a folder with an index.js file (like foo/bar/index.js), so that I can just do import bar from './foo/bar';

I can think of many examples. However, style is one of those things that it is easy to find arguments in both directions.

To quickly summarize my position:

  1. You want to name files the same way you will name the variable that will represent the file contents. The reason: it will promote consistent name of a resource throughout the code base.

  2. A path of the file will not always be visible. Consider example:

    ./formatters/hour.js
    ./formatters/minute.js
    ./formatters/time.js
    

    Now in ./time.js you want to import ./hour.js and ./minute.js.

    You cannot tell what ./hour and ./minute only by scanning the code (such is often the case when reviewing code fragments in PRs)

There are a few more arguments (such as promoting code re-usability), though this is getting off-topic.

This rule is opt-in and it will benefit developers who prefer explicit code naming conventions.

@mizchi
Copy link

mizchi commented Feb 18, 2017

I think we need default-export-match-filename too.

Example

// MyComponent.js
// pass
export default function MyComponent() { return ... }
// fail
export default function NotMyComponent() { return ... }

@ljqx
Copy link
Contributor

ljqx commented Feb 20, 2017

I think this rule (both export & import) can be configured by

  1. what to match
  • base file name
  • full path from project root (module id?)
  1. how to match
  • exact match (as Airbnb style)
  • filename/module id kebab case (friendly to Windows users, consistent with npm)
  • ...
  1. how to treat index.js
  • no special handling
  • the folder name is used as base file name or full path end

Then we can fit all requirements from different projects above, and leave the option to users.

For

import parseFloat from './parse/float';
import parseDate from './parse/date';
import formatFloat from './format/float';
import formatDate from './format/date';

It's valid for "full path" match, but not "base file name" match.

@ljharb
Copy link
Member

ljharb commented Feb 20, 2017

I'm not sure about your latter example - is parseFloat valid if the path is ./a/b/c/parse/float?

I think the rule should only ever use the base filename, or the base dirname if it's an index (the latter could be configurable, but should be on by default).

The rule would need to handle the most common cases; it's not practical for it to attempt to cover every conceivable pattern.

@ljqx
Copy link
Contributor

ljqx commented Feb 22, 2017

@ljharb
parseFloat is not valid when the path is ./a/b/c/parse/float for either “module specifier" match or "base filename" match. There will be only one parseFloat for the whole project, I think it's ok that it's at the top level of project as parseFloat.js (base filename or module specifier) or parse/float.js(module specifier) for grouping.

Matching base filename is not always good, especially for big project which has complicated features which is grouped by folders. As an example, there is one file named feature/subFeature/subSubFeature/Editor, we want it have a good name for readability/debuggability, then should we change the file to feature/subFeature/subSubFeature/FeatureSubFeatureSubSubFeatureEditor? Why duplicate the path again? That's same for parse/float too: parseFloat shows up with parse/float when importing import parseFloat from 'parse/float', later parseFloat is self-describing by its name, every code piece is readable, and no duplicate in path.

And, ./a/b/c/parse/float is not covered here :).

There maybe another configuration for all exceptions like import _ from 'lodash'.

@ljharb
Copy link
Member

ljharb commented Feb 22, 2017

Any argument for parse/float being parseFloat also argues for feature/parse/float being featureParseFloat.

@avegancafe
Copy link

Is there any movement on this? Or was it dropped :( I was looking around to see if this was a thing and stumbled upon this thread, would be awesome if even the rule was "imported value should have the same name as the export if the export is named"

@CarinaChenot
Copy link

Hello! I’m looking for a similar rule but with a different approach:

  1. A rule that gives an error if the filename is different from the default export name, basically this Proposal: default-import-match-filename #325 (comment)
  2. A rule that gives an error if the import name is different from the export name, basically what was described in Rule proposal: no-rename-default #1041, and what was done in Add match-default-export-name rule palantir/tslint#2117

(I’ve checked the current released rules, #1143 and #1476 and none of them seem to cover that)

I was wondering if implementing this proposal, means that #1041 won’t be implemented? Or can the 2 rules coexist?

@ljharb
Copy link
Member

ljharb commented Dec 29, 2020

@CarinaChenot I'd personally want to enforce that the filename matches the default export name, but I'd never want to enforce that users import it with the same name. However, many do want to impose that on their codebases.

Since the logic for "filename → export name" would need to be shared, it could be done as two separate rules, or as two options on one rule. Either way, I'm not interested in adding the import-side restriction unless the export-side one exists first/at the same time.

@dword-design
Copy link

@golopot How about publishing this rule as its own package?

@golopot
Copy link
Contributor

golopot commented Apr 21, 2021

@golopot How about publishing this rule as its own package?

I am not planning on it. But I am fine If anyone wants to create a package based on my pull request.

@minseoksuh
Copy link

minseoksuh commented Jul 25, 2021

Hi
@golopot, thanks for writing a great rule.

For educational purpose,
I have made a separate eslint-plugin that provides your 'default-import-match-filename' rule,
with 'default-export-match-filename' rule which I got from eslint-plugin-filenames

below is the link to my custom plugin:
https://github.com/minseoksuh/eslint-plugin-consistent-default-export-name

I made it abundantly clear in the README and in the code who the authors of the code are.

Would you mind if I publish it to npm and apply the rules to my company projects?

Thanks again

@golopot
Copy link
Contributor

golopot commented Jul 25, 2021

I am glad my work helps people! Surly you can publish it to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.